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

make check should not fail for release tarballs #14513

Closed
jellelicht opened this issue Jul 27, 2017 · 4 comments
Closed

make check should not fail for release tarballs #14513

jellelicht opened this issue Jul 27, 2017 · 4 comments
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.

Comments

@jellelicht
Copy link

jellelicht commented Jul 27, 2017 •
edited
Loading

PR #13658 introduced a hard-failing linter in case the linter dependencies are not included.

This happens to be the case for at least release 8.2.1, and as part of normal packaging procedures we run make check to verify that everything still works as it should. It seems reasonable to expect make check to only fail if there are problems with the code.

A linter not being available in the release tarball is IMHO not a reason for the included tests to fail.
@addaleax addaleax added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Jul 27, 2017
@Fishrock123
Copy link
Contributor

cc @gibfahn

@gibfahn
Copy link
Member

gibfahn commented Jul 28, 2017

If we don't include the linter in the release tarball then I agree that it doesn't make sense to fail tests. I guess the better change would be to change the linter check in https://github.com/nodejs/node/blob/master/Makefile#L915 from tools/eslint/lib/eslint.js to tools/eslint/.

Is there a way to detect that it's a release tarball (rather than an eslint upgrade gone wrong)?

@gibfahn gibfahn self-assigned this Sep 2, 2017
@jellelicht
Copy link
Author

@gibfahn I do not really see how development tooling is relevant to a release tarball.
IOW, why not just have a make target (make lint?) which is invoked as part of regular development QA, but not at all related to the release-related make check.

@gibfahn
Copy link
Member

gibfahn commented Sep 11, 2017

@jellelicht at the moment make check is just an alias for make test, there is no release-specific Makefile tooling:

node/Makefile

Line 117 in 640b206

check: test

gibfahn added a commit to gibfahn/node that referenced this issue Sep 16, 2017
@gibfahn
build: don't fail make test on source tarballs
341bcfe 
Tries to achieve the same effect as
nodejs#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

Fixes: nodejs#14513
@gibfahn gibfahn mentioned this issue Sep 16, 2017
Closed
2 tasks
@gibfahn gibfahn removed their assignment Sep 16, 2017
lukaszewczak pushed a commit to lukaszewczak/node that referenced this issue Sep 23, 2017
@gibfahn @lukaszewczak
build: don't fail make test on source tarballs
5d611c3 
Tries to achieve the same effect as
nodejs#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: nodejs#15441
Fixes: nodejs#14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 23, 2017
@gibfahn @addaleax
build: don't fail make test on source tarballs
cae02ce 
Tries to achieve the same effect as
nodejs/node#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: nodejs/node#15441
Fixes: nodejs/node#14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue Sep 25, 2017
@gibfahn @jasnell
build: don't fail make test on source tarballs
9685b9f 
Tries to achieve the same effect as
#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: #15441
Fixes: #14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 17, 2017
@gibfahn @MylesBorins
build: don't fail make test on source tarballs
8f1a586 
Tries to achieve the same effect as
#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: #15441
Fixes: #14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 25, 2017
@gibfahn @MylesBorins
build: don't fail make test on source tarballs
6c9a9ff 
Tries to achieve the same effect as
#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: #15441
Fixes: #14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jellelicht @addaleax @Fishrock123 @gibfahn