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: fix lint-md-build dependency #18981

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
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 nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 24, 2018
@joyeecheung
Copy link
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
Copy link
Member Author

@ChALkeR
Copy link
Member

ChALkeR commented Feb 24, 2018

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

/cc @refack @watilde

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2018
@joyeecheung
Copy link
Member Author

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

@watilde
Copy link
Member

watilde commented Feb 26, 2018

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

@ChALkeR
Copy link
Member

ChALkeR commented Feb 26, 2018

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

joyeecheung added a commit that referenced this pull request Feb 27, 2018
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
Copy link
Member Author

Landed in 0bff955, thanks!

addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018
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 MylesBorins mentioned this pull request Mar 6, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
Copy link
Member

jasnell commented Aug 17, 2018

Does this need to be backported to 8.x?

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 17, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lint-md on master failing
7 participants