FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: make tools/doc/node_modules non-phony by danbev · Pull Request #22189 · nodejs/node · GitHub

/ node Public

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

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:build_tools_doc_package.json
ConversationCommits (1)ChecksFiles changed
Closed

build: make tools/doc/node_modules non-phony#22189
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:build_tools_doc_package.json

Conversation

danbev commented Aug 8, 2018

Copy link
Copy Markdown
Contributor

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 commented Aug 8, 2018

Copy link
Copy Markdown
Collaborator

@danbev build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/460/pipeline

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

danbev commented Aug 8, 2018

Copy link
Copy Markdown
Contributor Author

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

Trott commented Aug 8, 2018

Copy link
Copy Markdown
Member

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

danbev mentioned this pull request Aug 9, 2018
Closed
2 tasks
joyeecheung approved these changes Aug 9, 2018
View reviewed changes
Comment thread Makefile Outdated

joyeecheung Aug 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is there any reason for the white space changes?

danbev Aug 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 #.

rubys Aug 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

danbev Aug 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

refack suggested changes Aug 9, 2018
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

danbev commented Aug 10, 2018

Copy link
Copy Markdown
Contributor Author

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

danbev commented Aug 10, 2018

Copy link
Copy Markdown
Contributor Author

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

build: make tools/doc/node_modules non-phony
b99e529 
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.
danbev force-pushed the build_tools_doc_package.json branch from e502580 to b99e529 Compare August 10, 2018 05:38
lundibundi mentioned this pull request Aug 10, 2018
Closed
refack approved these changes Aug 10, 2018
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

💯

danbev commented Aug 10, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 88bff82.

danbev closed this Aug 10, 2018
danbev added a commit that referenced this pull request Aug 10, 2018
build: make tools/doc/node_modules non-phony
88bff82 
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 <riclau@uk.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Aug 11, 2018
build: make tools/doc/node_modules non-phony
7135822 
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 <riclau@uk.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Aug 16, 2018
Closed
Closed
Closed
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
build: make tools/doc/node_modules non-phony
4520bb8 
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: nodejs/node#22189
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung approved these changes
richardlau richardlau approved these changes
+3 more reviewers
rubys rubys approved these changes
refack refack approved these changes
fhinkel fhinkel approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL