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

build: fix lint-md-build dependency by joyeecheung · Pull Request #18981 · nodejs/node · GitHub

/ node Public

build: fix lint-md-build dependency - #18981

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:fix-lint-md-build
ConversationCommits (1)ChecksFiles changed
Closed

build: fix lint-md-build dependency#18981
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:fix-lint-md-build

Conversation

joyeecheung commented Feb 24, 2018

Copy link
Copy Markdown
Member

Fixes: #18978

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

build

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

joyeecheung commented Feb 24, 2018

Copy link
Copy Markdown
Member Author

How to reproduce

# Clean up the deps
rm -rf tools/.docmdlintstamp tools/.miscmdlintstamp tools/remark-cli/node_modules tools/remark-preset-lint-node/node_modules
# Where the package.json didn't change
git revert a29089d7c866955616c0e363843017e9b9b2a736
make lint-md # Should print the hint to run make lint-md-build
make lint-md-build # Install
make lint-md # OK

git reset --hard HEAD~1 # Now the package.json changed
make lint-md # Issue in https://github.com/nodejs/node/issues/18978 shows up
make lint-md-build # Before this patch, this does nothing. After this patch, this install again
make lint-md # Before this patch, this still errors. After this patch, this runs OK

joyeecheung commented Feb 24, 2018

Copy link
Copy Markdown
Member Author

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

ChALkeR commented Feb 24, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Is there a reason why lint-md-build shouldn't be launched automatically as a dependency of lint-md now?

/cc @refack @watilde

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 26, 2018

joyeecheung commented Feb 26, 2018

Copy link
Copy Markdown
Member Author

@ChALkeR I think it's because make lint-md-build needs internet access..?

watilde commented Feb 26, 2018

Copy link
Copy Markdown
Member

^ That's true. We tried to avoid internet access.

ChALkeR commented Feb 26, 2018

Copy link
Copy Markdown
Member

Ah, understood. @joyeecheung, @watilde, thanks for clarification!

joyeecheung added a commit that referenced this pull request Feb 27, 2018
build: fix lint-md-build dependency
0bff955 
PR-URL: #18981
Fixes: #18978
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung commented Feb 27, 2018

Copy link
Copy Markdown
Member Author

Landed in 0bff955, thanks!

estrada9166 mentioned this pull request Mar 1, 2018
Closed
3 tasks
richardlau mentioned this pull request Mar 2, 2018
Closed
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018
build: fix lint-md-build dependency
28a5362 
PR-URL: nodejs#18981
Fixes: nodejs#18978
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Mar 6, 2018
Merged
This was referenced Mar 7, 2018
Closed
Closed
Closed
devsnek mentioned this pull request Mar 13, 2018
Closed
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
build: fix lint-md-build dependency
b5c962c 
PR-URL: nodejs#18981
Fixes: nodejs#18978
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Aug 17, 2018

Copy link
Copy Markdown
Member

Does this need to be backported to 8.x?

joyeecheung commented Aug 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

This should land cleanly on v8.x if #17964 is backported

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

ChALkeR ChALkeR approved these changes
jasnell jasnell approved these changes
watilde watilde approved these changes
+1 more reviewer
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

lint-md on master failing

7 participants

Back | FazBrowse Home | New Git URL