Add correct shared library naming on OS X #7687

sxa · 2016-07-12T18:10:58Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

Build system

Description of change

The build system currently creates a shared library on OS/X with the same name as on Linux
i.e. libnode.so.48. This is inconsistent with the conventions on OS/X which uses libnode.48.dylib
This change modifies the build process and install.py (used by make binary) to build with the
correct name on OS/X when the --shared configure parameter is used. Without these changes,
"make binary" with CONFIG_FLAGS=--shared fails on the install.py step

rvagg · 2016-07-13T02:49:07Z

sgtm, @nodejs/build?

bnoordhuis · 2016-07-13T03:26:28Z

tools/install.py

-      # see the _InstallableTargetInstallPath function.
-      output_prefix += 'lib.target/'
-      output_file = 'lib' + output_file + '.so.' + get_version()
+      if sys.platform== 'darwin':


Style: space before ==. You can fold the if into the preceding else, saves a level of indent.

(In case you're not a pythonista, the keyword is elif.)

bnoordhuis · 2016-07-13T03:28:25Z

It's kind of nasty to duplicate the logic in two places. It's arguably better to set a variable in configure and use that.

sxa · 2016-07-13T10:22:55Z

True @bnoordhuis - I was just following the same pattern that was already in place for splitting Windows/Linux. I'll look at modifying it to use a variable for the shlib filename as you suggest (although it may still be cleaner overall to have the darwin-specific switch for the directory prefix in install.py instead of having that passed through) I'm looking separately at AIX too which generally uses .a as the suffix so abstracting this out as you suggest will make that simpler.

sxa · 2016-07-15T15:44:58Z

@bnoordhuis Updates done to have the shared lib suffixes differences only into configure. In theory we could abstract this over windows as well and use a prefix variable too, but I'd propose that be done separately if we want it.

bnoordhuis · 2016-07-16T11:27:59Z

configure

@@ -842,6 +842,11 @@ def configure_node(o):
  o['variables']['node_shared'] = b(options.shared)
  o['variables']['node_module_version'] = int(getmoduleversion.get_version())

+  if sys.platform == 'darwin':
+    o['variables']['shlib_suffix'] = '%s.dylib' % o['variables']['node_module_version']


Can you make sure lines stay < 80 columns?

ChALkeR · 2016-07-16T11:55:26Z

OS/X
OSX

Nit: «macOS». Or «OS X».

bnoordhuis · 2016-07-18T12:20:18Z

tools/install.py

-      # see the _InstallableTargetInstallPath function.
-      output_prefix += 'lib.target/'
-      output_file = 'lib' + output_file + '.so.' + get_version()
+      output_file = 'lib' + output_file + "." + variables.get('shlib_suffix')


Style nit: '.'

bnoordhuis · 2016-07-18T12:20:31Z

LGTM with nit. Can you rebase and squash?

EDIT: Note that the commit log should conform to the guidelines from CONTRIBUTING.md.

sxa · 2016-07-18T17:38:51Z

Should all be good now - have adjusted the commit log too.

ChALkeR · 2016-07-18T18:06:24Z

The commit message should have a build: prefix on the first line.

jasnell · 2016-08-04T20:45:27Z

@nodejs/build ... any further thoughts on this?
It LGTM

bnoordhuis · 2016-08-04T20:52:47Z

Still LGTM but s/Add/add/ on the first line of the commit log. Whoever lands this can fix that up.

jasnell · 2016-08-04T21:32:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/3529/

mhdawson · 2016-08-05T01:55:30Z

LGTM, @jasnell you planning to land ? I'll try to take a look tomorrow AM and land if you have not already.

jasnell · 2016-08-05T02:02:35Z

Go for it. I'm stepping away from the laptop for a while tonight

On Thursday, August 4, 2016, Michael Dawson notifications@github.com
wrote:

LGTM, @jasnell https://github.com/jasnell you planning to land ? I'll
try to take a look tomorrow AM and land if you have not already.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7687 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eQhzEGrPO9GlNaqzBH-B0woVNVYsks5qcpgZgaJpZM4JKqr2
.

mhdawson · 2016-08-05T13:44:33Z

CI run had some failures, don't think they are related, but just in case: https://ci.nodejs.org/job/node-test-pull-request/3538/

mhdawson · 2016-08-05T14:07:06Z

@sxa555, Was just about to land but seems that the windows shared lib patch was landed and there is now a conflict. Can you rebase and then let me know and I'll land.

jbergstroem · 2016-08-05T14:14:37Z

LGTM with commit message nit (ref Bens comment)

The build system currently creates a shared library on OS X with the same name as on Linux i.e. libnode.so.48. This is inconsistent with the conventions on OS X which uses libnode.48.so This commit changes the build process and install.py (used by make binary) to build with the correct name on OS X when the --shared configure parameter is used.

The build system currently creates a shared library on OS X with the same name as on Linux i.e. libnode.so.48. This is inconsistent with the conventions on OS X which uses libnode.48.so This commit changes the build process and install.py (used by make binary) to build with the correct name on OS X when the --shared configure parameter is used. PR-URL: #7687 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson · 2016-08-09T16:02:44Z

Landed as a722205

The build system currently creates a shared library on OS X with the same name as on Linux i.e. libnode.so.48. This is inconsistent with the conventions on OS X which uses libnode.48.so This commit changes the build process and install.py (used by make binary) to build with the correct name on OS X when the --shared configure parameter is used. PR-URL: #7687 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Conflicts: node.gyp

MylesBorins · 2016-09-30T18:19:03Z

This is not landing cleanly on v4.x. Would someone be willing to do a manual backport?

sxa · 2016-10-04T18:35:01Z

@thealphanerd Yeah I'll take a look at it - I have a set of changes kicking around that I've used to get it working on V4 already. It was on my list to push them back when I get some time :-)

MylesBorins · 2016-10-24T16:41:14Z

ping @sxa555

sxa · 2016-10-26T16:06:57Z

@thealphanerd ping accepted :-) We don't currently have any of the shared library stuff in v4.x so it's not a trivial matter of just doing the name change for OS/X. I do want to get it into V4 though and have got a bit of time for it now (and as per earlier comment I have had it working - just need to get a PR in for it).

MylesBorins · 2016-10-26T17:23:05Z

@sxa555 awesome! Keep me in the loop if you need any support at all

WIP: Add soname & fix make install PR-URL: nodejs#6994 Ref: nodejs#9385 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor@indutny.com> The build system currently creates a shared library on OS X with the same name as on Linux i.e. libnode.so.48. This is inconsistent with the conventions on OS X which uses libnode.48.dylib This commit changes the build process and install.py (used by make binary) to build with the correct name on OS X when the --shared configure parameter is used. PR-URL: nodejs#7687 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs#7687 Ref: nodejs#9385 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor@indutny.com> The build system currently creates a shared library on OS X with the same name as on Linux i.e. libnode.so.48. This is inconsistent with the conventions on OS X which uses libnode.48.dylib This commit changes the build process and install.py (used by make binary) to build with the correct name on OS X when the --shared configure parameter is used. PR-URL: nodejs#7687 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

WIP: Add soname & fix make install PR-URL: nodejs#6994 Ref: nodejs#9385 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor@indutny.com> The build system currently creates a shared library on OS X with the same name as on Linux i.e. libnode.so.48. This is inconsistent with the conventions on OS X which uses libnode.48.dylib This commit changes the build process and install.py (used by make binary) to build with the correct name on OS X when the --shared configure parameter is used. PR-URL: nodejs#7687 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 12, 2016

addaleax added the macos Issues and PRs related to the macOS platform / OSX. label Jul 12, 2016

bnoordhuis reviewed Jul 13, 2016
View reviewed changes

sxa force-pushed the sharedlib-osx branch from 6eb2f26 to fad176c Compare July 15, 2016 15:16

bnoordhuis reviewed Jul 16, 2016
View reviewed changes

bnoordhuis reviewed Jul 18, 2016
View reviewed changes

sxa force-pushed the sharedlib-osx branch 3 times, most recently from 2ca7ded to 4edb4cb Compare July 18, 2016 17:29

sxa force-pushed the sharedlib-osx branch from 4edb4cb to 4dfcae3 Compare July 18, 2016 18:18

sxa changed the title ~~Add correct shared library naming on OS/X~~ Add correct shared library naming on OS X Jul 19, 2016

sxa force-pushed the sharedlib-osx branch from 4dfcae3 to 9e99c37 Compare August 7, 2016 12:10

mhdawson closed this Aug 9, 2016

cjihrig mentioned this pull request Aug 10, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

MylesBorins added the lts-watch-v4.x label Sep 30, 2016

MylesBorins added this to the v4.6.2 milestone Oct 24, 2016

MylesBorins modified the milestones: v4.7.0, v4.6.2 Oct 26, 2016

sxa mentioned this pull request Oct 31, 2016

Backport --shared to V4.x codebase to allow building as a shared library / DLL #9385

Closed

2 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 14, 2016

MylesBorins removed this from the 4.7.0 milestone Nov 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add correct shared library naming on OS X #7687

Add correct shared library naming on OS X #7687

sxa commented Jul 12, 2016 •

edited

Loading

rvagg commented Jul 13, 2016

bnoordhuis Jul 13, 2016

bnoordhuis commented Jul 13, 2016

sxa commented Jul 13, 2016

sxa commented Jul 15, 2016

bnoordhuis Jul 16, 2016

ChALkeR commented Jul 16, 2016 •

edited

Loading

bnoordhuis Jul 18, 2016

bnoordhuis commented Jul 18, 2016 •

edited

Loading

sxa commented Jul 18, 2016

ChALkeR commented Jul 18, 2016

jasnell commented Aug 4, 2016

bnoordhuis commented Aug 4, 2016

jasnell commented Aug 4, 2016

mhdawson commented Aug 5, 2016

jasnell commented Aug 5, 2016

mhdawson commented Aug 5, 2016

mhdawson commented Aug 5, 2016

jbergstroem commented Aug 5, 2016

mhdawson commented Aug 9, 2016

MylesBorins commented Sep 30, 2016

sxa commented Oct 4, 2016

MylesBorins commented Oct 24, 2016

sxa commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

Add correct shared library naming on OS X #7687

Add correct shared library naming on OS X #7687

Conversation

sxa commented Jul 12, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

rvagg commented Jul 13, 2016

bnoordhuis Jul 13, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jul 13, 2016

sxa commented Jul 13, 2016

sxa commented Jul 15, 2016

bnoordhuis Jul 16, 2016

Choose a reason for hiding this comment

ChALkeR commented Jul 16, 2016 • edited Loading

bnoordhuis Jul 18, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jul 18, 2016 • edited Loading

sxa commented Jul 18, 2016

ChALkeR commented Jul 18, 2016

jasnell commented Aug 4, 2016

bnoordhuis commented Aug 4, 2016

jasnell commented Aug 4, 2016

mhdawson commented Aug 5, 2016

jasnell commented Aug 5, 2016

mhdawson commented Aug 5, 2016

mhdawson commented Aug 5, 2016

jbergstroem commented Aug 5, 2016

mhdawson commented Aug 9, 2016

MylesBorins commented Sep 30, 2016

sxa commented Oct 4, 2016

MylesBorins commented Oct 24, 2016

sxa commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

sxa commented Jul 12, 2016 •

edited

Loading

ChALkeR commented Jul 16, 2016 •

edited

Loading

bnoordhuis commented Jul 18, 2016 •

edited

Loading