Revert "build,test: make building addon tests less fragile" - broken source tarballs #18287

rvagg · 2018-01-22T04:17:04Z

@bnoordhuis this is yours, it was landed in between the last two nightlies. Current one is broken, previous one (commit e9c6dcd) is fine.

make: *** No rule to make target 'deps/uv/test/benchmark-async-pummel.c', needed by 'out/Release/node'.  Stop.

Simulate this in the full source tree with rm -rf deps/uv/test/ prior to make and you'll get the error. It's because we $(RM) -r $(TARNAME)/deps/uv/{docs,samples,test} for the source tarball but we've introduced a dependency on test/* and basically everything else that gets built thanks to the new .deps functionality introduced in gyp.

Not sure what the way forward is here, I understand what the change is intending to do, but it doesn't work for distributable source now. Do you want to come up with a solution or should we revert while you take some time with it?

bnoordhuis

I'll have a look but I'm good with reverting in the interim.

rvagg · 2018-01-23T02:48:45Z

addons don't build for tests, so this isn't quite ready

rvagg · 2018-01-23T11:06:03Z

reverted both commits and the addons fail to build properly in CI, giving up for another day

BridgeAR · 2018-01-23T11:32:10Z

@bnoordhuis do you think you can come up with a fix soon? In that case that might be the best to do?

richardlau · 2018-01-23T15:41:36Z

reverted both commits and the addons fail to build properly in CI, giving up for another day

#17407 landed as five commits.

bnoordhuis · 2018-01-23T19:00:49Z

FWIW, I can't reproduce locally. Here is what I did:

$ make node-v10.0.0.tar XZ=0
$ tar xfz node-v10.0.0.tar.gz
$ cd node-v10.0.0/
$ ./configure && make -j8 test
# ...
make -s lint
Running JS linter...
Cannot find module '@shinnn/eslint-config-node'
Referenced from: /home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/node_modules/is-resolvable/package.json
Error: Cannot find module '@shinnn/eslint-config-node'
Referenced from: /home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/node_modules/is-resolvable/package.json
    at ModuleResolver.resolve (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/util/module-resolver.js:74:19)
    at resolve (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/config/config-file.js:471:25)
    at load (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/config/config-file.js:542:26)
    at configExtends.reduceRight (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/config/config-file.js:421:36)
    at Array.reduceRight (<anonymous>)
    at applyExtends (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/config/config-file.js:403:28)
    at loadFromDisk (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/config/config-file.js:514:22)
    at Object.load (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/config/config-file.js:550:20)
    at Config.getLocalConfigHierarchy (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/config.js:228:44)
    at Config.getConfigHierarchy (/home/bnoordhuis/src/master/node-v10.0.0/tools/node_modules/eslint/lib/config.js:180:43)
Makefile:1012: recipe for target 'lint-js' failed
make[2]: *** [lint-js] Error 1
Makefile:1081: recipe for target 'lint' failed
make[1]: *** [lint] Error 2
Makefile:229: recipe for target 'test' failed
make: *** [test] Error 2

It doesn't fail because of anything introduced by #17407, as far as I can tell. node, cctest and all the add-ons build just fine. The failure looks like (but may not be exactly) #17098.

joyeecheung · 2018-01-23T19:04:12Z

@bnoordhuis Is it because the linters are run prior to building addons? For testing source tarballs make test-only is more suitable.

bnoordhuis · 2018-01-23T19:09:09Z

I figured out what the non-reproduce issue was: the $(RM) action didn't work on my machine. It does with this patch:

diff --git a/Makefile b/Makefile
index 6efb50950e..f62182d62e 100644
--- a/Makefile
+++ b/Makefile
@@ -831,7 +831,9 @@ $(TARBALL): release-only $(NODE_EXE) doc
        cp -r out/doc/api/* $(TARNAME)/doc/api/
        $(RM) -r $(TARNAME)/deps/v8/{test,samples,tools/profviz,tools/run-tests.py}
        $(RM) -r $(TARNAME)/doc/images # too big
-       $(RM) -r $(TARNAME)/deps/uv/{docs,samples,test}
+       $(RM) -r $(TARNAME)/deps/uv/docs
+       $(RM) -r $(TARNAME)/deps/uv/samples
+       $(RM) -r $(TARNAME)/deps/uv/test
        $(RM) -r $(TARNAME)/deps/openssl/openssl/{doc,demos,test}
        $(RM) -r $(TARNAME)/deps/zlib/contrib # too big, unused
        $(RM) -r $(TARNAME)/.{editorconfig,git*,mailmap}

I'll see what I can do.

bnoordhuis · 2018-01-23T19:28:42Z

Is it because the linters are run prior to building addons?

@joyeecheung No, the add-ons built fine (but I clipped that from the output.)

gibfahn · 2018-01-23T19:50:41Z

I figured out what the non-reproduce issue was: the $(RM) action didn't work on my machine. It does with this patch:

That seems odd. So the other similar lines work, e.g.

$(RM) -r $(TARNAME)/deps/v8/{test,samples,tools/profviz,tools/run-tests.py}

but not that one?

Make it easier for Node.js to ship libuv in its tarballs without also including the test suite. Node.js already does so but recent changes to its build system complicate that. Kills two birds with one stone: it helps out Node.js and it makes it harder for us to introduce hidden dependencies between the library and the test suite. Refs: nodejs/node#18287

bnoordhuis · 2018-01-23T19:59:05Z

@gibfahn None of them work but the one I highlighted was the reason for me not being able to reproduce.

gibfahn · 2018-01-24T00:04:44Z

@bnoordhuis so does libuv/libuv#1725 fix the issue? If so I guess we'll still need to revert until we can get an updated libuv into master.

rvagg · 2018-01-24T10:49:29Z

This gets all green now except for a single unrelated failure on Windows https://ci.nodejs.org/job/node-test-commit/15643/

I've reverted 4 commits from the original PR of 5:

[2cb9e2a6f7] - build,tools: check freshness of doc addons (Ben Noordhuis) #17407
[d9b59def72] - build,test: make building addon tests less fragile (Ben Noordhuis) #17407
[c6682636be] - tools: simplify tools/doc/addon-verify.js (Ben Noordhuis) #17407
[920c13203d] - tools: teach gyp to write an 'all deps' rule (Ben Noordhuis) #17407

EDIT(gibfahn): For posterity the comment that wasn't reverted is

143b235 - test: fix flaky cluster unix socket test

@bnoordhuis are you OK with this, is it the best path forward in the short-term while you sort it out from the libuv end upward?

bnoordhuis · 2018-01-24T12:44:43Z

@rvagg Go ahead. I just hope people don't land too many conflicting changes in the interim.

This reverts commit 2cb9e2a. Reverted along with d9b59de as this introduces freshness checks that are too stringent without the comprehensive dependency checking of introduced in d9b59de so `make test` won't work with this.

This reverts commit d9b59de. Breaks downloadable source tarball builds as we remove some files prior to creating a tarball but those files are included in the comprehensive list of dependencies listed in .deps.

This reverts commit c668263. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs.

This reverts commit 920c132. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs.

rvagg · 2018-01-24T21:24:40Z

landed, thanks for the reviews folks

This reverts commit 2cb9e2a. Reverted along with d9b59de as this introduces freshness checks that are too stringent without the comprehensive dependency checking of introduced in d9b59de so `make test` won't work with this. Ref: #17407 PR-URL: #18287 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This reverts commit d9b59de. Breaks downloadable source tarball builds as we remove some files prior to creating a tarball but those files are included in the comprehensive list of dependencies listed in .deps. Ref: #17407 PR-URL: #18287 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This reverts commit c668263. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs. Ref: #17407 PR-URL: #18287 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This reverts commit 920c132. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs. Ref: #17407 PR-URL: #18287 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Make it easier for Node.js to ship libuv in its tarballs without also including the test suite. Node.js already does so but recent changes to its build system complicate that. Kills two birds with one stone: it helps out Node.js and it makes it harder for us to introduce hidden dependencies between the library and the test suite. PR-URL: libuv#1725 Refs: nodejs/node#18287 Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins · 2018-02-20T22:29:33Z

I don't think the original landed on v9.x and am setting this to don't land

This reverts commit 2cb9e2a. Reverted along with d9b59de as this introduces freshness checks that are too stringent without the comprehensive dependency checking of introduced in d9b59de so `make test` won't work with this. Ref: nodejs#17407 PR-URL: nodejs#18287 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This reverts commit d9b59de. Breaks downloadable source tarball builds as we remove some files prior to creating a tarball but those files are included in the comprehensive list of dependencies listed in .deps. Ref: nodejs#17407 PR-URL: nodejs#18287 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This reverts commit c668263. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs. Ref: nodejs#17407 PR-URL: nodejs#18287 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This reverts commit 920c132. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs. Ref: nodejs#17407 PR-URL: nodejs#18287 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels Jan 22, 2018

rvagg requested a review from bnoordhuis January 22, 2018 04:20

bnoordhuis approved these changes Jan 22, 2018

View reviewed changes

BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 22, 2018

gireeshpunathil approved these changes Jan 22, 2018

View reviewed changes

richardlau approved these changes Jan 22, 2018

View reviewed changes

jasnell approved these changes Jan 22, 2018

View reviewed changes

rvagg force-pushed the rvagg/revert-d9b59def72c718aaad3eefb6bf43f409ccefe4d2 branch 2 times, most recently from 583687d to 3c78c37 Compare January 23, 2018 10:50

This was referenced Jan 23, 2018

build: split off tests into separate gyp file libuv/libuv#1725

Merged

build: fix rm commands in tarball rule #18332

Closed

rvagg force-pushed the rvagg/revert-d9b59def72c718aaad3eefb6bf43f409ccefe4d2 branch from 023c8ca to e8372f1 Compare January 24, 2018 03:29

rvagg added 3 commits January 25, 2018 08:14

Revert "build,tools: check freshness of doc addons"

4d705e6

This reverts commit 2cb9e2a. Reverted along with d9b59de as this introduces freshness checks that are too stringent without the comprehensive dependency checking of introduced in d9b59de so `make test` won't work with this.

Revert "build,test: make building addon tests less fragile"

a6051eb

This reverts commit d9b59de. Breaks downloadable source tarball builds as we remove some files prior to creating a tarball but those files are included in the comprehensive list of dependencies listed in .deps.

Revert "tools: simplify tools/doc/addon-verify.js"

9134772

This reverts commit c668263. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs.

Revert "tools: teach gyp to write an 'all deps' rule"

cd9fd23

This reverts commit 920c132. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs.

rvagg force-pushed the rvagg/revert-d9b59def72c718aaad3eefb6bf43f409ccefe4d2 branch from e8372f1 to cd9fd23 Compare January 24, 2018 21:15

rvagg closed this Jan 24, 2018

rvagg mentioned this pull request Jan 24, 2018

build,test: make building addon tests less fragile #17407

Closed

MylesBorins added the dont-land-on-v9.x label Feb 20, 2018

codebytere added the dont-land-on-v8.x label Aug 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "build,test: make building addon tests less fragile" - broken source tarballs #18287

Revert "build,test: make building addon tests less fragile" - broken source tarballs #18287

rvagg commented Jan 22, 2018

bnoordhuis left a comment

rvagg commented Jan 23, 2018

rvagg commented Jan 23, 2018

BridgeAR commented Jan 23, 2018

richardlau commented Jan 23, 2018

bnoordhuis commented Jan 23, 2018

joyeecheung commented Jan 23, 2018

bnoordhuis commented Jan 23, 2018

bnoordhuis commented Jan 23, 2018

gibfahn commented Jan 23, 2018

bnoordhuis commented Jan 23, 2018

gibfahn commented Jan 24, 2018

rvagg commented Jan 24, 2018 •

edited by gibfahn

Loading

bnoordhuis commented Jan 24, 2018

rvagg commented Jan 24, 2018

MylesBorins commented Feb 20, 2018

Revert "build,test: make building addon tests less fragile" - broken source tarballs #18287

Revert "build,test: make building addon tests less fragile" - broken source tarballs #18287

Conversation

rvagg commented Jan 22, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

rvagg commented Jan 23, 2018

rvagg commented Jan 23, 2018

BridgeAR commented Jan 23, 2018

richardlau commented Jan 23, 2018

bnoordhuis commented Jan 23, 2018

joyeecheung commented Jan 23, 2018

bnoordhuis commented Jan 23, 2018

bnoordhuis commented Jan 23, 2018

gibfahn commented Jan 23, 2018

bnoordhuis commented Jan 23, 2018

gibfahn commented Jan 24, 2018

rvagg commented Jan 24, 2018 • edited by gibfahn Loading

bnoordhuis commented Jan 24, 2018

rvagg commented Jan 24, 2018

MylesBorins commented Feb 20, 2018

rvagg commented Jan 24, 2018 •

edited by gibfahn

Loading