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

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

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

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

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 fad176cCompare July 15, 2016 15:16

bnoordhuis reviewed Jul 16, 2016
View reviewed changes

Comment threadconfigure Outdated
bnoordhuisJul 16, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make sure lines stay < 80 columns?

bnoordhuis reviewed Jul 18, 2016
View reviewed changes

Comment threadtools/install.py Outdated
bnoordhuisJul 18, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: '.'

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

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

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

sxa force-pushed the sharedlib-osx branch from 4dfcae3 to 9e99c37Compare 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.2Oct 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

Uh oh!

Uh oh!

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!

bnoordhuisJul 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!

bnoordhuisJul 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!

bnoordhuisJul 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

sxa commented Jul 12, 2016•
edited
Loading

ChALkeR commented Jul 16, 2016•
edited
Loading

bnoordhuis commented Jul 18, 2016•
edited
Loading