build: fix shared installing target by yorkie · Pull Request #15148 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This patch fixes the target location with --shared flag, we should install the lib under $node_prefix/lib

how about:

action([output_prefix + output_file], ('bin/' if 'false' == variables.get('node_shared') else 'lib/) + output_file)

that only affect one line.

Nice suggestion! Merged to one statement but still 2 lines because of the 80 chars per line rule :)

Will be easier on the eyes when written as an if/else statement. That's also the commonest style in this file and will also let you keep it <= 80 columns.

Like this?

if 'false' == variables.get('node_shared'):
  action([output_prefix + output_file], 'bin/' + output_file)
else:
  action([output_prefix + output_file], 'lib/' + output_file)

Ping @bnoordhuis

@yorkie that looks like what Ben was suggesting, could you update this branch with that code?

Could someone take a look at this? This is a little patch to fix the vital issue!

@nodejs/build PTAL

Still Ping @bnoordhuis :(

Ping @nodejs/build again

Ping @nodejs/tsc

@yorkie that looks like what Ben was suggesting, could you update this branch with that code?

Ok, @gibfahn @bnoordhuis Updated :)

Nit addressed

cc/ @nodejs/python

Ping @XadillaX LGTY now?

I think this is good to land if CI is green.

CI: https://ci.nodejs.org/job/node-test-commit/12619/

Landed in 6975c49

PR-URL: #15148 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#15148 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #15148 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs/node#15148 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #15148 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

The npm install rules had a hidden dependency on the `node` binary install rule creating the `$PREFIX/bin` directory. Because with `./configure --shared` no binary is created, the rule subsequently failed. Fix that by creating the directory before creating the symlinks to the npm and npx scripts. (Whether it makes sense to install npm without a `node` binary is a separate question. This commit is not taking positions. :-)) Regression introduced in commit ed8c89a ("build: fix shared installing target") which, as the commit log indicates, was itself a bug fix for the `./configure --shared` install. Fixes: nodejs#16437 Refs: nodejs#15148

PR-URL: #15148 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

The npm install rules had a hidden dependency on the `node` binary install rule creating the `$PREFIX/bin` directory. Because with `./configure --shared` no binary is created, the rule subsequently failed. Fix that by creating the directory before creating the symlinks to the npm and npx scripts. (Whether it makes sense to install npm without a `node` binary is a separate question. This commit is not taking positions. :-)) Regression introduced in commit ed8c89a ("build: fix shared installing target") which, as the commit log indicates, was itself a bug fix for the `./configure --shared` install. PR-URL: #16438 Fixes: #16437 Ref: #15148 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

The npm install rules had a hidden dependency on the `node` binary install rule creating the `$PREFIX/bin` directory. Because with `./configure --shared` no binary is created, the rule subsequently failed. Fix that by creating the directory before creating the symlinks to the npm and npx scripts. (Whether it makes sense to install npm without a `node` binary is a separate question. This commit is not taking positions. :-)) Regression introduced in commit ed8c89a ("build: fix shared installing target") which, as the commit log indicates, was itself a bug fix for the `./configure --shared` install. PR-URL: nodejs/node#16438 Fixes: nodejs/node#16437 Ref: nodejs/node#15148 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

The npm install rules had a hidden dependency on the `node` binary install rule creating the `$PREFIX/bin` directory. Because with `./configure --shared` no binary is created, the rule subsequently failed. Fix that by creating the directory before creating the symlinks to the npm and npx scripts. (Whether it makes sense to install npm without a `node` binary is a separate question. This commit is not taking positions. :-)) Regression introduced in commit ed8c89a ("build: fix shared installing target") which, as the commit log indicates, was itself a bug fix for the `./configure --shared` install. PR-URL: #16438 Fixes: #16437 Ref: #15148 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

The npm install rules had a hidden dependency on the `node` binary install rule creating the `$PREFIX/bin` directory. Because with `./configure --shared` no binary is created, the rule subsequently failed. Fix that by creating the directory before creating the symlinks to the npm and npx scripts. (Whether it makes sense to install npm without a `node` binary is a separate question. This commit is not taking positions. :-)) Regression introduced in commit ed8c89a ("build: fix shared installing target") which, as the commit log indicates, was itself a bug fix for the `./configure --shared` install. PR-URL: nodejs/node#16438 Fixes: nodejs/node#16437 Ref: nodejs/node#15148 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

build: fix shared installing target

ba7bbce

yorkie requested a review from stefanmb September 2, 2017 12:33

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Sep 2, 2017

XadillaX reviewed Sep 3, 2017

View reviewed changes

fix nits, merging to one statement

f49c18c

bnoordhuis previously requested changes Sep 3, 2017

View reviewed changes

fixup

528cf2d

BridgeAR approved these changes Sep 24, 2017

View reviewed changes

gibfahn approved these changes Sep 24, 2017

View reviewed changes

bnoordhuis approved these changes Sep 25, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

BridgeAR closed this Sep 28, 2017

yorkie deleted the fix/shared-install branch September 28, 2017 06:52

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the land-on-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

bnoordhuis mentioned this pull request Oct 24, 2017

build: fix npm install with --shared #16438

Closed

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

build: fix shared installing target by yorkie · Pull Request #15148 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: fix shared installing target - #15148

build: fix shared installing target#15148yorkie wants to merge 3 commits intonodejs:masternodejs/node:masterfrom rokid:fix/shared-installrokid/node:fix/shared-installCopy head branch name to clipboard

Conversation

yorkie commented Sep 2, 2017

Checklist

Uh oh!

XadillaX Sep 3, 2017

Choose a reason for hiding this comment

Uh oh!

yorkie Sep 3, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Sep 3, 2017

Choose a reason for hiding this comment

Uh oh!

yorkie Sep 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yorkie Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn Sep 22, 2017

Choose a reason for hiding this comment

Uh oh!

yorkie commented Sep 7, 2017

Uh oh!

BridgeAR commented Sep 7, 2017

Uh oh!

yorkie commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

BridgeAR commented Sep 24, 2017

Uh oh!

yorkie commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Sep 24, 2017

Uh oh!

yorkie commented Sep 26, 2017

Uh oh!

gibfahn commented Sep 26, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

build: fix shared installing target#15148
yorkie wants to merge 3 commits into
nodejs:masterfrom
rokid:fix/shared-install

yorkie Sep 3, 2017 •
edited
Loading

yorkie commented Sep 10, 2017 •
edited
Loading

yorkie commented Sep 24, 2017 •
edited
Loading