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.

Sign up for GitHub

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

Jump to bottom

build: make linter failures fail test-doc target #30012

Closed
wants to merge 1 commit into from
Closed

build: make linter failures fail test-doc target #30012

wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

Linter failures in the test-doc make target were not failing the
build if the subsequent doctools test passed as its exit code
wasn't being preserved.

Make the lint target a dependency of test-doc so that it is
outside of the node_use_openssl guard -- its own dependencies
have their own guards where necessary and the targets that don't
require an available node (e.g. the C++ linters) will be allowed
to run.

Refs: #29999 (comment)

I'd overlooked this when reviewing #28199 where the regression was introduced.

cc @nodejs/build-files @danbev @sam-github

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

@richardlau
build: make linter failures fail test-doc target
1663f13 
Linter failures in the `test-doc` make target were not failing the
build if the subsequent `doctools` test passed as its exit code
wasn't being preserved.

Make the `lint` target a dependency of `test-doc` so that it is
outside of the `node_use_openssl` guard -- its own dependencies
have their own guards where necessary and the targets that don't
require an available node (e.g. the C++ linters) will be allowed
to run.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 17, 2019
@richardlau richardlau mentioned this pull request Oct 17, 2019
Closed
3 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@sam-github
Copy link
Contributor

@watson you hit this in #30008 (comment)

@richardlau richardlau mentioned this pull request Oct 17, 2019
Closed
5 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 17, 2019
danbev
danbev approved these changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@danbev danbev left a comment

Choose a reason for hiding this comment

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

Sorry about that 😞

@Trott
Copy link
Member

Trott commented Oct 22, 2019

Landed in b5b7e58

Trott pushed a commit that referenced this pull request Oct 22, 2019
@richardlau @Trott
build: make linter failures fail test-doc target
b5b7e58 
Linter failures in the `test-doc` make target were not failing the
build if the subsequent `doctools` test passed as its exit code
wasn't being preserved.

Make the `lint` target a dependency of `test-doc` so that it is
outside of the `node_use_openssl` guard -- its own dependencies
have their own guards where necessary and the targets that don't
require an available node (e.g. the C++ linters) will be allowed
to run.

PR-URL: #30012
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@Trott Trott closed this Oct 22, 2019
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@richardlau @MylesBorins
build: make linter failures fail test-doc target
b680905 
Linter failures in the `test-doc` make target were not failing the
build if the subsequent `doctools` test passed as its exit code
wasn't being preserved.

Make the `lint` target a dependency of `test-doc` so that it is
outside of the `node_use_openssl` guard -- its own dependencies
have their own guards where necessary and the targets that don't
require an available node (e.g. the C++ linters) will be allowed
to run.

PR-URL: #30012
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@richardlau @MylesBorins
build: make linter failures fail test-doc target
19a983c 
Linter failures in the `test-doc` make target were not failing the
build if the subsequent `doctools` test passed as its exit code
wasn't being preserved.

Make the `lint` target a dependency of `test-doc` so that it is
outside of the `node_use_openssl` guard -- its own dependencies
have their own guards where necessary and the targets that don't
require an available node (e.g. the C++ linters) will be allowed
to run.

PR-URL: #30012
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@richardlau @targos
build: make linter failures fail test-doc target
6c31cc6 
Linter failures in the `test-doc` make target were not failing the
build if the subsequent `doctools` test passed as its exit code
wasn't being preserved.

Make the `lint` target a dependency of `test-doc` so that it is
outside of the `node_use_openssl` guard -- its own dependencies
have their own guards where necessary and the targets that don't
require an available node (e.g. the C++ linters) will be allowed
to run.

PR-URL: #30012
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
@richardlau @targos
build: make linter failures fail test-doc target
c306917 
Linter failures in the `test-doc` make target were not failing the
build if the subsequent `doctools` test passed as its exit code
wasn't being preserved.

Make the `lint` target a dependency of `test-doc` so that it is
outside of the `node_use_openssl` guard -- its own dependencies
have their own guards where necessary and the targets that don't
require an available node (e.g. the C++ linters) will be allowed
to run.

PR-URL: #30012
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@targos targos mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
@richardlau @targos
build: make linter failures fail test-doc target
032c23d 
Linter failures in the `test-doc` make target were not failing the
build if the subsequent `doctools` test passed as its exit code
wasn't being preserved.

Make the `lint` target a dependency of `test-doc` so that it is
outside of the `node_use_openssl` guard -- its own dependencies
have their own guards where necessary and the targets that don't
require an available node (e.g. the C++ linters) will be allowed
to run.

PR-URL: #30012
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@richardlau @nodejs-github-bot @sam-github @Trott @danbev @jasnell @cjihrig @gireeshpunathil