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

test: Node 8.9.0 cannot 'make test' when building from source code #16650

Closed
BobCochran opened this issue Oct 31, 2017 · 13 comments
Closed

test: Node 8.9.0 cannot 'make test' when building from source code #16650

BobCochran opened this issue Oct 31, 2017 · 13 comments
Labels
test Issues and PRs related to the tests.

Comments

@BobCochran
Copy link

I downloaded the source code tarball node-v8.9.0.tar.gz to build node on a Fedora 26 workstation. 'make test' fails at this point:

`Building addon /home/bcochranc/Downloads/node-v8.9.0/test/addons-napi/test_warning/
make[2]: Entering directory '/home/bcochranc/Downloads/node-v8.9.0/test/addons-napi/test_warning/build'
CC(target) Release/obj.target/test_warning/test_warning.o
SOLINK_MODULE(target) Release/obj.target/test_warning.node
COPY Release/test_warning.node
CC(target) Release/obj.target/test_warning2/test_warning2.o
SOLINK_MODULE(target) Release/obj.target/test_warning2.node
COPY Release/test_warning2.node
make[2]: Leaving directory '/home/bcochranc/Downloads/node-v8.9.0/test/addons-napi/test_warning/build'
touch test/addons-napi/.buildstamp
make doc-only
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN [email protected] No repository field.
npm WARN [email protected] No license field.

added 3 packages and updated 1 package in 0.872s
/bin/sh: out/doc/api/debugger.html: No such file or directory
make[1]: *** [Makefile:579: out/doc/api/debugger.html] Error 1
make: *** [Makefile:215: test] Error 2
`

I was last able to make test successfully with version 8.7.0. Node releases since then have failed when attempting to make test.

@mscdex mscdex added test Issues and PRs related to the tests. v8.x labels Oct 31, 2017
@vsemozhetbyt
Copy link
Contributor

@joyeecheung, is it test doc related?

@drewfish
Copy link
Contributor

In my company we run tests using gmake test-ci-native test-ci-js.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 1, 2017

@BobCochran I've tried it on Ubuntu and looks like if you run make doc-only, it will fail on the first attempt but will work the second time. Can you try running it again?

I'll investigate why this fails on the first attempt..Also this works on Mac so I think there are probably some differences in gnu make.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 1, 2017

diff --git a/Makefile b/Makefile
index c2f872daed..f7fc5b676d 100644
--- a/Makefile
+++ b/Makefile
@@ -544,7 +544,8 @@ apidoc_dirs = out/doc out/doc/api/ out/doc/api/assets

 apiassets = $(subst api_assets,api/assets,$(addprefix out/,$(wildcard doc/api_assets/*)))

-doc-only: $(apidocs_html) $(apidocs_json)
+doc-targets: $(apidocs_html) $(apidocs_json)
+doc-only: | install-yaml doc-targets
 doc: $(NODE_EXE) doc-only

 $(apidoc_dirs):
@@ -561,15 +562,16 @@ gen-json = tools/doc/generate.js --format=json $< > $@
 gen-html = tools/doc/generate.js --node-version=$(FULLVERSION) --format=html \
 			--template=doc/template.html --analytics=$(DOCS_ANALYTICS) $< > $@

-gen-doc =	\
+install-yaml:
 	[ -e tools/doc/node_modules/js-yaml/package.json ] || \
 		[ -e tools/eslint/node_modules/js-yaml/package.json ] || \
 		if [ -x $(NODE) ]; then \
 			cd tools/doc && ../../$(NODE) ../../$(NPM) install; \
 		else \
 			cd tools/doc && node ../../$(NPM) install; \
-		fi;\
-	[ -x $(NODE) ] && $(NODE) $(1) || node $(1)
+		fi;
+
+gen-doc = [ -x $(NODE) ] && $(NODE) $(1) || node $(1)

 out/doc/api/%.json: doc/api/%.md
 	@$(call gen-doc, $(gen-json))

There is probably some problems in order of dependencies, this patch seems to work for me on Ubuntu, I will open a PR shortly

@joyeecheung
Copy link
Member

By the way, make test is more for developers now because it runs linters and builds docs. If you only want to test if the binary is working, you can run make test-only.

@BobCochran
Copy link
Author

BobCochran commented Nov 1, 2017 via email

@BobCochran
Copy link
Author

@joyeecheung I tried your suggestion by running 'make test' a second time, and it worked fine. My first execution of 'make test' ended in an error, at which point I created this issue to report it in. Then following your advice I ran 'make test' a second time. That worked fine. This is the output I received at the console.

If you would like me to assist in resolving this issue in any way (such as to test a patch) please let me know.

Thank you for telling me that I can run make test-only if I just want to execute tests. I am really a javascript user of node.js, rather than a developer of node.js itself, so I appreciate the tips.

make_test_results.txt

@brettsheffield
Copy link

Running make test prior to running make doc results in this error:

added 3 packages and updated 1 package in 1.717s
/bin/sh: out/doc/api/console.html: No such file or directory
Makefile:579: recipe for target 'out/doc/api/console.html' failed
make[1]: *** [out/doc/api/console.html] Error 1
Makefile:213: recipe for target 'test' failed
make: *** [test] Error 2

After running make doc, make test succeeds.

I suggest an update to https://github.com/nodejs/node/blob/master/BUILDING.md#building-nodejs-on-supported-platforms to clarify that docs need to be built prior to running make test

@BobCochran
Copy link
Author

BobCochran commented Nov 17, 2017 via email

@joyeecheung
Copy link
Member

joyeecheung commented Nov 17, 2017

@BobCochran @brettsheffield Yes, and it's supposed to work without running make doc first, this should be fixed by #16661.

Also this seems specific to GNU make v4.x, I suspect it's the conditionals in gen-doc that are interpreted differently.

@joyeecheung
Copy link
Member

BTW, make test on the v9.2.0 tarball still does not pass at the moment, because the source tarball does not include eslint and we don't install that in the Makefile. I'll open an issue

@joyeecheung
Copy link
Member

hmm..I think I understand why this failed with GNU make 4.x now, in 3.x

out/doc/%: doc/%
	@cp -r $< $@

takes precedence over

out/doc/api/%.json: doc/api/%.md
	@$(call gen-doc, $(gen-json))

# check if ./node is actually set, else use user pre-installed binary
out/doc/api/%.html: doc/api/%.md
	@$(call gen-doc, $(gen-html))

while in 4.x, the latter will be called first. So in 3.x, make would not even try to generate the docs because they are already in doc/api and can just be copied over. We need to somehow fix the dependency chain to get make 4.x run the former first again.

@BobCochran
Copy link
Author

BobCochran commented Nov 17, 2017 via email

joyeecheung added a commit that referenced this issue Nov 20, 2017
PR-URL: #17100
Fixes: #16650
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
PR-URL: #16661
Fixes: #16650
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
PR-URL: #17100
Fixes: #16650
Reviewed-By: Refael Ackermann <[email protected]>
gibfahn pushed a commit that referenced this issue Dec 19, 2017
PR-URL: #16661
Fixes: #16650
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
gibfahn pushed a commit that referenced this issue Dec 19, 2017
PR-URL: #17100
Fixes: #16650
Reviewed-By: Refael Ackermann <[email protected]>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
PR-URL: #16661
Fixes: #16650
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
PR-URL: #17100
Fixes: #16650
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
6 participants