Add correct shared library naming on OS X by sxa · Pull Request #7687 · nodejs/node · GitHub

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

sgtm, @nodejs/build?

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.)

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.

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.

@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.

Can you make sure lines stay < 80 columns?

OS/X
OSX

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

Style nit: '.'

LGTM with nit. Can you rebase and squash?

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

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

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

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

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

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

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

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
.

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/

@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.

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>

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

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

@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 :-)

ping @sxa555

@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).

@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

Add correct shared library naming on OS X by sxa · Pull Request #7687 · nodejs/node · GitHub

Uh oh!

Uh oh!

Add correct shared library naming on OS X - #7687

Add correct shared library naming on OS X#7687sxa wants to merge 1 commit intonodejs:masternodejs/node:masterfrom sxa:sharedlib-osxsxa/node:sharedlib-osxCopy head branch name to clipboard

Conversation

sxa commented Jul 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

rvagg commented Jul 13, 2016

Uh oh!

bnoordhuis Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jul 13, 2016

Uh oh!

sxa commented Jul 13, 2016

Uh oh!

sxa commented Jul 15, 2016

Uh oh!

bnoordhuis Jul 16, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Jul 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis Jul 18, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jul 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sxa commented Jul 18, 2016

Uh oh!

ChALkeR commented Jul 18, 2016

Uh oh!

jasnell commented Aug 4, 2016

Uh oh!

bnoordhuis commented Aug 4, 2016

Uh oh!

jasnell commented Aug 4, 2016

Uh oh!

mhdawson commented Aug 5, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

mhdawson commented Aug 5, 2016

Uh oh!

mhdawson commented Aug 5, 2016

Uh oh!

jbergstroem commented Aug 5, 2016

Uh oh!

mhdawson commented Aug 9, 2016

Uh oh!

MylesBorins commented Sep 30, 2016

Uh oh!

sxa commented Oct 4, 2016

Uh oh!

MylesBorins commented Oct 24, 2016

Uh oh!

sxa commented Oct 26, 2016

Uh oh!

MylesBorins commented Oct 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Add correct shared library naming on OS X#7687
sxa wants to merge 1 commit into
nodejs:masterfrom
sxa:sharedlib-osx

sxa commented Jul 12, 2016 •
edited
Loading

ChALkeR commented Jul 16, 2016 •
edited
Loading

bnoordhuis commented Jul 18, 2016 •
edited
Loading