build: make 'make test' run linter and build docs #22031

rubys · 2018-07-30T16:18:55Z

Before this change, the comment for 'make test' read as follows:

## Runs default tests, linters, and builds docs

What it actually did was:

run build-addons twice
run build-addons-napi twice
run cctest once
run jstest once
not run linters
not build docs

Arguably, one could make the comments match the code, or the code match the
comments. Given that there is a separate make target named test-only that
does exactly what test did, I went the other way.

Along the way, other fixes were made:

build-addons and build-addons-napi were listed as .PHONY, but had
no commands defined. I removed the .PHONY markers and moved them to
test/addons/.buildstamp and test/addons-napi/.buildstamp where they
would have the desired effect.
tools/doc/node_modules was listed as .PHONY and did not depend on
tools/doc/package.json. This caused it to unnecessary run every time
it was referenced.

After these changes, make test will:

run build-addons once
run build-addons-napi once
run cctest once
run jstest once
run linters once
build docs once

Checklist

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

Before this change, the comment for 'make test' read as follows: ## Runs default tests, linters, and builds docs What it actually did was: * run build-addons twice * run build-addons-napi twice * run cctest once * run jstest once * not run linters * not build docs Arguably, one could make the comments match the code, or the code match the comments. Given that there is a separate make target named `test-only` that does exactly what `test` did, I went the other way. Along the way, other fixes were made: * `build-addons` and `build-addons-napi` were listed as `.PHONY`, but had no commands defined. I removed the `.PHONY` markers and moved them to `test/addons/.buildstamp` and `test/addons-napi/.buildstamp` where they would have the desired effect. * `tools/doc/node_modules` was listed as `.PHONY` and did not depend on `tools/doc/package.json`. This caused it to unnecessary run every time it was referenced. After these changes, `make test` will: * run build-addons once * run build-addons-napi once * run cctest once * run jstest once * run linters once * build docs once

vsemozhetbyt · 2018-07-30T16:43:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16084/

BridgeAR · 2018-07-30T19:13:48Z

Makefile

@@ -276,7 +281,7 @@ test-only: all  ## For a quick test, does not run linter or build docs.
 	$(MAKE) build-addons
 	$(MAKE) build-addons-napi
 	$(MAKE) cctest
-	$(MAKE) jstest
+	$(MAKE) test-js


Would it not be better to remove the addon parts from this task and keep jstest? Just the cctest would have to move below. The same for the other entries.

This make rule is designed to run a number of steps serially. Depending on jstest would enable make to run these commands in parallel; and in particular, before all is completed. Note that the definition of build-addons does not require $(NODE_EXE) to be present.

build-addons: | $(NODE_EXE) test/addons/.buildstamp

refack · 2018-07-30T19:47:13Z

build-addons and build-addons-napi were listed as .PHONY, but had
no commands defined. I removed the .PHONY markers and moved them to
test/addons/.buildstamp and test/addons-napi/.buildstamp where they
would have the desired effect.

I understand that empirically this change achieves the desired result, but I'm not sure I understand why. AFAICT .PHONY targets are for targets that are "commands" and don't actuly output files. But in this case test/addons/.buildstamp and test/addons-napi/.buildstamp are files, while build-addons and build-addons-napi aren't.

/CC @nodejs/build-files

rubys · 2018-07-30T20:45:22Z

@refack at this point, the @touch $@ statements in these two rules are superfluous. Once a rule is marked as .PHONY, it matters not whether there is a corresponding file or what its timestamp might be.

richardlau · 2018-07-31T01:03:01Z

Makefile

@@ -356,7 +362,6 @@ test/addons/.buildstamp: config.gypi \
 		"$$PWD/deps/npm/node_modules/node-gyp/bin/node-gyp.js" "$$PWD/test/addons"
 	touch $@

-.PHONY: build-addons


build-addons should remain .PHONY as it does not refer to a file.

richardlau · 2018-07-31T01:03:44Z

Makefile

@@ -387,7 +393,6 @@ test/addons-napi/.buildstamp: config.gypi \
 		"$$PWD/test/addons-napi"
 	touch $@

-.PHONY: build-addons-napi


build-addons-napi should remain .PHONY as it does not refer to a file.

richardlau · 2018-07-31T01:10:58Z

Makefile

@@ -342,6 +347,7 @@ ADDONS_BINDING_SOURCES := \
 	$(filter-out test/addons/??_*/*.cc, $(wildcard test/addons/*/*.cc)) \
 	$(filter-out test/addons/??_*/*.h, $(wildcard test/addons/*/*.h))

+.PHONY: test/addons/.buildstamp


If we're going to make this target .PHONY, I would remove the touch so it's no longer a file and probably rename the target so it doesn't look like a file either.

richardlau · 2018-07-31T01:11:06Z

Makefile

@@ -374,6 +379,7 @@ ADDONS_NAPI_BINDING_SOURCES := \
 	$(filter-out test/addons-napi/??_*/*.cc, $(wildcard test/addons-napi/*/*.cc)) \
 	$(filter-out test/addons-napi/??_*/*.h, $(wildcard test/addons-napi/*/*.h))

+.PHONY: test/addons-napi/.buildstamp


If we're going to make this target .PHONY, I would remove the touch so it's no longer a file and probably rename the target so it doesn't look like a file either.

Trott · 2018-08-05T04:58:45Z

Not sure when it changed, but make test used to run linters like the comment says. It would be great to have that behavior restored before Code + Learn in October. It will be an annoyance otherwise.

maclover7 · 2018-08-11T20:24:19Z

ping @rubys

BridgeAR · 2018-09-06T00:08:54Z

This needs a rebase and the comments have to be looked at.

Trott · 2018-11-20T21:10:12Z

@rubys Still working on this?

BridgeAR · 2019-03-10T00:57:24Z

Closing due to long inactivity. @rubys please feel free to reopen or open a new PR in case you would like to continue working on this.

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 30, 2018

BridgeAR reviewed Jul 30, 2018

View reviewed changes

refack added the tools Issues and PRs related to the tools directory. label Jul 30, 2018

richardlau requested changes Jul 31, 2018

View reviewed changes

rubys mentioned this pull request Aug 10, 2018

build: make tools/doc/node_modules non-phony #22189

Closed

2 tasks

maclover7 added the wip Issues and PRs that are still a work in progress. label Aug 11, 2018

BridgeAR closed this Mar 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: make 'make test' run linter and build docs #22031

build: make 'make test' run linter and build docs #22031

rubys commented Jul 30, 2018

vsemozhetbyt commented Jul 30, 2018

BridgeAR Jul 30, 2018

rubys Jul 30, 2018

refack commented Jul 30, 2018

rubys commented Jul 30, 2018

richardlau Jul 31, 2018

richardlau Jul 31, 2018

richardlau Jul 31, 2018

richardlau Jul 31, 2018

Trott commented Aug 5, 2018

maclover7 commented Aug 11, 2018

BridgeAR commented Sep 6, 2018

Trott commented Nov 20, 2018

BridgeAR commented Mar 10, 2019

build: make 'make test' run linter and build docs #22031

build: make 'make test' run linter and build docs #22031

Conversation

rubys commented Jul 30, 2018

Checklist

vsemozhetbyt commented Jul 30, 2018

BridgeAR Jul 30, 2018

Choose a reason for hiding this comment

rubys Jul 30, 2018

Choose a reason for hiding this comment

refack commented Jul 30, 2018

rubys commented Jul 30, 2018

richardlau Jul 31, 2018

Choose a reason for hiding this comment

richardlau Jul 31, 2018

Choose a reason for hiding this comment

richardlau Jul 31, 2018

Choose a reason for hiding this comment

richardlau Jul 31, 2018

Choose a reason for hiding this comment

Trott commented Aug 5, 2018

maclover7 commented Aug 11, 2018

BridgeAR commented Sep 6, 2018

Trott commented Nov 20, 2018

BridgeAR commented Mar 10, 2019