build: Add /opt/freeware/... to AIX library path #10128

sxa · 2016-12-05T18:29:38Z

Checklist

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

Affected core subsystem(s)

build

Description of change

At the moment the library search path encoded into the AIX binary has the full path to whether the GNU libstdc++ and libgcc_s were installed on the machine. Unless the user has the exact same package/version as was used on the build machine, the node binary will not be able to pick them up by default, and the user will have to set the LIBPATH variable themselves to the location on their machine.

On the assumption that most people will be picking up their is of libstdc++/libgcc_s from either the Bull Freeware site or the IBM AIX toolbox, this PR adjusts the search path within the node binary to point to /opt/freeware/lib (or lib64) to find this library. The current packages from Bull Freeware/AIX toolbox symlinks those libraries into those locations, so it is far more likely to work out of the box than the current system. For reference, here is the library search path within the current node 7.2.0 binary:

/opt/freeware/lib/gcc/powerpc-ibm-aix6.1.0.0/4.8.5/pthread/ppc64:/opt/freeware/lib/gcc/powerpc-ibm-aix6.1.0.0/4.8.5/../../../pthread/ppc64:/opt/freeware/lib/gcc/powerpc-ibm-aix6.1.0.0/4.8.5:/opt/freeware/lib/gcc/powerpc-ibm-aix6.1.0.0/4.8.5/../../..:/usr/lib:/lib

This change squashes that to be /usr/lib:/lib:/opt/freeware/lib (or lib64).

Considerations for reviewers:

I have added /opt/freeware/lib{64} to the end instead of the start of the search path. if someone happens to have a version of the GNU C++ libraries in /usr/lib they will be picked up in preference to any in /opt/freeware/lib64
If the user has multiple versions of the GNU C++ runtimes libraries installed (unlikely you'd hope!) and an old one is linked into /opt/freeware/lib64 then it may be too old. This is the only situation in which we might be worse off - if the user has multiple versions, and has the exact version in the same place as the one encoded into the current binaries.
We could add /usr/local/lib{64} as well which would be the most common location for anyone that has a self-compiled version of gcc available, but I suspect that to ease support and discourage building their own GNU C++ libraries we should leave it as-is.

mhdawson · 2016-12-05T21:18:08Z

Looks like you have some stray commits in the PR. Those from Nov 23. I expect only the last one whould be in the PR.

mhdawson

Change itself looks ok, but need to fix up commits included.

sxa · 2016-12-06T14:10:59Z

Done - I'd added some unlanded commits into the master branch I'd based this off. SIde effect to submitting the PR quickly before I was leaving he office for the day :)

mhdawson

LGTM

mhdawson · 2016-12-06T19:02:47Z

CI run to validate: https://ci.nodejs.org/job/node-test-pull-request/5267/

gibfahn · 2016-12-09T20:54:08Z

LGTM, this seems like a sensible default, and anything that means less fiddling with the LIBPATH sounds great.

Windows CI failure looks unrelated.

sxa · 2016-12-12T18:55:29Z

There's an follow-up issue with this that I'll need to address - it's not correctly picking up the threaded version of libstdc++ on all machines - please do not land yet.

sxa · 2016-12-13T19:44:06Z

Follow-up issue resolved - it wasn't pointing at the location of the pthread variants of the GNU C++ runtime libraries. If someone could run this through CI again that would be appreciated ...

bnoordhuis

LGTM but the status line should be <= 50 characters and the convention is to not uppercase (i.e., 'modify' instead of 'Modify'.)

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

bnoordhuis · 2016-12-13T21:24:58Z

node.gyp

@@ -936,6 +936,12 @@
            }, {
              'type': 'executable',
            }],
+            ['target_arch=="ppc64"', {
+              'ldflags': ['-Wl,-blibpath:/usr/lib:/lib:/opt/freeware/lib/pthread/ppc64'],


Perhaps the tiniest of nits: if you break after the [, the line fits in 80 columns.

Done - made the change to both versions to make them consistent

sxa · 2016-12-14T19:00:05Z

@bnoordhuis The first line of the commit message was <50 characters - it was only the title of this PR that was longer in order to be a bit more descriptive - I've changed it now, but are the titles of PRs always expected to be that short too?

gibfahn · 2016-12-14T19:37:36Z

@sxa555 Agreed that your commit message is already <50 chars, but it is still capitalised (s/Add/add)

To ease the use of the AIX binaries, add /opt/freeware/lib/pthread{/ppc64} into the search path encoded into the library, so that any version the user has installed from the common download locations will work out of the box without having to explicitly set LIBPATH in their environment.

jasnell · 2016-12-23T05:47:06Z

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

italoacasas · 2016-12-29T02:26:59Z

@gibfahn are you going to land this ?

To ease the use of the AIX binaries, add /opt/freeware/lib/pthread{/ppc64} into the search path encoded into the library, so that any version the user has installed from the common download locations will work out of the box without having to explicitly set LIBPATH in their environment. PR-URL: #10128 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn · 2016-12-29T10:00:28Z

Landed in e407478

To ease the use of the AIX binaries, add /opt/freeware/lib/pthread{/ppc64} into the search path encoded into the library, so that any version the user has installed from the common download locations will work out of the box without having to explicitly set LIBPATH in their environment. PR-URL: #10128 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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

mscdex added the aix Issues and PRs related to the AIX platform. label Dec 5, 2016

mhdawson requested changes Dec 5, 2016

View reviewed changes

sxa force-pushed the aixlibpath branch from 309d967 to a27af4f Compare December 6, 2016 14:09

mhdawson approved these changes Dec 6, 2016

View reviewed changes

jbergstroem approved these changes Dec 6, 2016

View reviewed changes

gibfahn approved these changes Dec 9, 2016

View reviewed changes

sxa force-pushed the aixlibpath branch from a27af4f to 62277cc Compare December 13, 2016 19:38

nodejs-github-bot added lts-watch-v6.x labels Dec 13, 2016

sxa force-pushed the aixlibpath branch from 62277cc to 5c6eff0 Compare December 13, 2016 19:41

nodejs-github-bot added lts-watch-v6.x labels Dec 13, 2016

sxa force-pushed the aixlibpath branch from 5c6eff0 to 6f2b0c7 Compare December 13, 2016 19:49

nodejs-github-bot added lts-watch-v6.x labels Dec 13, 2016

bnoordhuis approved these changes Dec 13, 2016

View reviewed changes

bnoordhuis reviewed Dec 13, 2016

View reviewed changes

sxa changed the title ~~build: Modify library search path on AIX to include /opt/freeware/lib~~ build: Add /opt/freeware/... to AIX library path Dec 14, 2016

sxa force-pushed the aixlibpath branch from 6f2b0c7 to 26cabdb Compare December 14, 2016 18:56

nodejs-github-bot added lts-watch-v6.x labels Dec 14, 2016

sxa force-pushed the aixlibpath branch from 26cabdb to 024a4bf Compare December 15, 2016 11:35

nodejs-github-bot added lts-watch-v6.x labels Dec 15, 2016

jasnell approved these changes Dec 23, 2016

View reviewed changes

gibfahn self-assigned this Dec 23, 2016

gibfahn closed this Dec 29, 2016

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

This was referenced Jan 24, 2017

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

Merged

richardlau mentioned this pull request Jan 11, 2019

LIBPATH issue in AIX node binary #25444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Add /opt/freeware/... to AIX library path #10128

build: Add /opt/freeware/... to AIX library path #10128

sxa commented Dec 5, 2016

mhdawson commented Dec 5, 2016

mhdawson left a comment

sxa commented Dec 6, 2016 •

edited

Loading

mhdawson left a comment

mhdawson commented Dec 6, 2016

gibfahn commented Dec 9, 2016

sxa commented Dec 12, 2016

sxa commented Dec 13, 2016

bnoordhuis left a comment

bnoordhuis Dec 13, 2016

sxa Dec 14, 2016

sxa commented Dec 14, 2016

gibfahn commented Dec 14, 2016

jasnell commented Dec 23, 2016

italoacasas commented Dec 29, 2016

gibfahn commented Dec 29, 2016

build: Add /opt/freeware/... to AIX library path #10128

build: Add /opt/freeware/... to AIX library path #10128

Conversation

sxa commented Dec 5, 2016

Checklist

Affected core subsystem(s)

Description of change

mhdawson commented Dec 5, 2016

mhdawson left a comment

Choose a reason for hiding this comment

sxa commented Dec 6, 2016 • edited Loading

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Dec 6, 2016

gibfahn commented Dec 9, 2016

sxa commented Dec 12, 2016

sxa commented Dec 13, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Dec 13, 2016

Choose a reason for hiding this comment

sxa Dec 14, 2016

Choose a reason for hiding this comment

sxa commented Dec 14, 2016

gibfahn commented Dec 14, 2016

jasnell commented Dec 23, 2016

italoacasas commented Dec 29, 2016

gibfahn commented Dec 29, 2016

sxa commented Dec 6, 2016 •

edited

Loading