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: make tools/doc/node_modules non-phony #22189

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 8, 2018

This commit makes the target tools/doc/node_modules a non-phony target
and also adds tools/doc/package.json as a prerequisite to it to avoid
running it unnecessary. This is currently causing the target
test/addons/.docbuildstamp to be always be executed as it has
tools/doc/node_modules as a prerequisite.

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 8, 2018
@danbev
Copy link
Contributor Author

danbev commented Aug 8, 2018

@Trott
Copy link
Member

Trott commented Aug 8, 2018

/ping @rubys @richardlau @nodejs/build-files

Makefile Outdated
@@ -262,26 +262,26 @@ jstest: build-addons build-addons-napi ## Runs addon tests and JS tests
.PHONY: test
# This does not run tests of third-party libraries inside deps.
test: all ## Runs default tests, linters, and builds docs.
# Build the addons before running the tests so the test results
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason for the white space changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the indentation the comments are sent to the shell and displayed as:

# Build the addons before running the tests so the test results

My reasoning was that this was a mistake and that if we wanted the comments to be displayed the echo command would be used instead and without the #.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @danbev . I'll also note that the addons are built twice and the linters aren't run. See #22031 (I got distracted by work and now am on vacation). If this pull request could clean some of this up, that would be great. Otherwise, I'll take a look at cleaning up the rest when I get back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just removed the commit from the PR but I can create a separate one with this change so that this one can be merged.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

I'd prefer we keep the comments in the shell scope for this PR.

@danbev danbev force-pushed the build_tools_doc_package.json branch from 1191a66 to e502580 Compare August 10, 2018 03:35
@danbev
Copy link
Contributor Author

danbev commented Aug 10, 2018

@danbev
Copy link
Contributor Author

danbev commented Aug 10, 2018

I'd prefer we keep the comments in the shell scope for this PR.

@refack I've removed that commit from the PR. If CI comes back green could you remove you change request as I'd like to merge this? Thanks

This commit makes the target tools/doc/node_modules a non-phony target
and also adds tools/doc/package.json as a prerequisite to it to avoid
running it unnecessary. This is currently causing the target
test/addons/.docbuildstamp to be always be executed as it has
tools/doc/node_modules as a prerequisite.
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯

@danbev
Copy link
Contributor Author

danbev commented Aug 10, 2018

Landed in 88bff82.

@danbev danbev closed this Aug 10, 2018
danbev added a commit that referenced this pull request Aug 10, 2018
This commit makes the target tools/doc/node_modules a non-phony target
and also adds tools/doc/package.json as a prerequisite to it to avoid
running it unnecessary. This is currently causing the target
test/addons/.docbuildstamp to be always be executed as it has
tools/doc/node_modules as a prerequisite.

PR-URL: #22189
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Aug 11, 2018
This commit makes the target tools/doc/node_modules a non-phony target
and also adds tools/doc/package.json as a prerequisite to it to avoid
running it unnecessary. This is currently causing the target
test/addons/.docbuildstamp to be always be executed as it has
tools/doc/node_modules as a prerequisite.

PR-URL: #22189
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
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
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants