Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: use npm ci #22399

Merged
merged 1 commit into from
Aug 23, 2018
Merged

build: use npm ci #22399

merged 1 commit into from
Aug 23, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Aug 19, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Ref: #21802
Ref: #21490

@refack refack requested a review from rubys August 19, 2018 14:15
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Aug 19, 2018
@refack
Copy link
Contributor Author

refack commented Aug 19, 2018

/CC @nodejs/build-files @nodejs/documentation

@refack
Copy link
Contributor Author

refack commented Aug 19, 2018

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2018
Trott
Trott previously approved these changes Aug 19, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM. (Avoiding official sign-off because I don't know all the details of how some of this stuff is invoked as well as others know it, and this already has two sign-offs, so it doesn't need mine.)

@Trott Trott dismissed their stale review August 19, 2018 16:34

meant to leave a comment...

@rubys
Copy link
Member

rubys commented Aug 19, 2018

Meta: just so you know what you are getting with my approval. Essentially, it means that I'm saying "it looks plausible to me that it would work", and "should it break, I will help fix it".

I've been programming continuously since the mid 70s, so I have accumulated a working knowledge of batch files, make files, C, ES5, and the like. That doesn't mean that I have a deep understanding of npm, gyp, acorn, node's repl, etc. What you have seen there is only evidence that I'm a fast learner.

@refack
Copy link
Contributor Author

refack commented Aug 19, 2018

Meta: just so you know what you are getting with my approval. Essentially, it means that I'm saying "it looks plausible to me that it would work", and "should it break, I will help fix it".

Ditto 😄
There is definite emergent complexity in our build scripts, one that only empirical experiments can uncover.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

(On a side note, I think we should also use --ignore-scripts in the installation commands..)

@refack
Copy link
Contributor Author

refack commented Aug 21, 2018

(On a side note, I think we should also use --ignore-scripts in the installation commands..)

I'm not sure about that... Especially since node-gyp build is triggered as the default install script... I trust @zkat that npm ci was optimally designed.

Anyway I leave that to a future PR.

@refack
Copy link
Contributor Author

refack commented Aug 22, 2018

linux/ubuntu1404 fail is #20628

@refack
Copy link
Contributor Author

refack commented Aug 22, 2018

* remove obsolete `node_modules/js-yaml/package.json` target
* remove `@touch` since `npm ci` is always destructive

PR-URL: nodejs#22399
Refs: nodejs#21802
Refs: nodejs#21490
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@refack
Copy link
Contributor Author

refack commented Aug 23, 2018

Landed in d320511

@refack refack deleted the npm-ci branch August 23, 2018 01:16
@addaleax
Copy link
Member

$ make format-cpp-build
cd tools/clang-format && if [ -x /home/xxxx/src/node/master/./node ] && [ -e /home/xxxx/src/node/master/./node ]; then /home/xxxx/src/node/master/./node /home/xxxx/src/node/master/./deps/npm/bin/npm-cli.js ci; elif [ -x `which node` ] && [ -e `which node` ] && [ `which node` ]; then `which node` /home/xxxx/src/node/master/./deps/npm/bin/npm-cli.js ci; else echo "No available node, cannot run \"node /home/xxxx/src/node/master/./deps/npm/bin/npm-cli.js ci\""; exit 1; fi;
npm ERR! cipm can only install packages with an existing package-lock.json or npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or later to generate it, then try again.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/xxxx/.npm/_logs/2018-08-23T14_41_43_532Z-debug.log
Makefile:1182: recipe for target 'format-cpp-build' failed
make: *** [format-cpp-build] Error 1

🙁

@refack refack mentioned this pull request Aug 23, 2018
2 tasks
targos pushed a commit that referenced this pull request Aug 24, 2018
* remove obsolete `node_modules/js-yaml/package.json` target
* remove `@touch` since `npm ci` is always destructive

PR-URL: #22399
Refs: #21802
Refs: #21490
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
* remove obsolete `node_modules/js-yaml/package.json` target
* remove `@touch` since `npm ci` is always destructive

PR-URL: #22399
Refs: #21802
Refs: #21490
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants