build: fix shared installing target #15148

yorkie · 2017-09-02T12:33:45Z

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

XadillaX · 2017-09-03T07:55:47Z

tools/install.py

@@ -132,8 +133,7 @@ def files(action):
      # in its source - see the _InstallableTargetInstallPath function.
      if sys.platform != 'darwin':
        output_prefix += 'lib.target/'
-
-  action([output_prefix + output_file], 'bin/' + output_file)
+    action([output_prefix + output_file], 'bin/' + output_file)


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

bnoordhuis · 2017-09-03T15:30:24Z

tools/install.py

@@ -133,7 +133,8 @@ def files(action):
      if sys.platform != 'darwin':
        output_prefix += 'lib.target/'

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


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?

yorkie · 2017-09-07T16:16:59Z

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

BridgeAR · 2017-09-07T20:12:23Z

@nodejs/build PTAL

yorkie · 2017-09-10T14:14:20Z

Still Ping @bnoordhuis :(

BridgeAR · 2017-09-20T12:45:25Z

Ping @nodejs/build again

BridgeAR · 2017-09-24T15:28:48Z

Ping @nodejs/tsc

yorkie · 2017-09-24T18:03:25Z

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

Ok, @gibfahn @bnoordhuis Updated :)

Nit addressed

gibfahn · 2017-09-24T19:10:57Z

cc/ @nodejs/python

yorkie · 2017-09-26T04:26:30Z

Ping @XadillaX LGTY now?

gibfahn · 2017-09-26T12:26:34Z

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

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

BridgeAR · 2017-09-28T05:05:16Z

Landed in 6975c49

PR-URL: #15148 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#15148 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #15148 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#15148 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #15148 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: fix shared installing target #15148

build: fix shared installing target #15148

yorkie commented Sep 2, 2017

XadillaX Sep 3, 2017

yorkie Sep 3, 2017

bnoordhuis Sep 3, 2017

yorkie Sep 3, 2017 •

edited

Loading

yorkie Sep 5, 2017

gibfahn Sep 22, 2017

yorkie commented Sep 7, 2017

BridgeAR commented Sep 7, 2017

yorkie commented Sep 10, 2017 •

edited

Loading

BridgeAR commented Sep 20, 2017

BridgeAR commented Sep 24, 2017

yorkie commented Sep 24, 2017 •

edited

Loading

gibfahn commented Sep 24, 2017

yorkie commented Sep 26, 2017

gibfahn commented Sep 26, 2017

BridgeAR commented Sep 28, 2017

build: fix shared installing target #15148

build: fix shared installing target #15148

Conversation

yorkie commented Sep 2, 2017

Checklist

XadillaX Sep 3, 2017

Choose a reason for hiding this comment

yorkie Sep 3, 2017

Choose a reason for hiding this comment

bnoordhuis Sep 3, 2017

Choose a reason for hiding this comment

yorkie Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

yorkie Sep 5, 2017

Choose a reason for hiding this comment

gibfahn Sep 22, 2017

Choose a reason for hiding this comment

yorkie commented Sep 7, 2017

BridgeAR commented Sep 7, 2017

yorkie commented Sep 10, 2017 • edited Loading

BridgeAR commented Sep 20, 2017

BridgeAR commented Sep 24, 2017

yorkie commented Sep 24, 2017 • edited Loading

gibfahn commented Sep 24, 2017

yorkie commented Sep 26, 2017

gibfahn commented Sep 26, 2017

BridgeAR commented Sep 28, 2017

yorkie Sep 3, 2017 •

edited

Loading

yorkie commented Sep 10, 2017 •

edited

Loading

yorkie commented Sep 24, 2017 •

edited

Loading