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: make linter targets silent #12423

Closed
wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.

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

build

--

cc @nodejs/build

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 15, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Doesn't bother me personally but LGTM.

@gibfahn
Copy link
Member

gibfahn commented Apr 15, 2017

I'd be somewhat -1 on this as I often copy the linter commands when I want to edit them, e.g. to add --fix and auto-fix linter errors. I think it's helpful for people running make lint to understand what's actually being run.

@thefourtheye
Copy link
Contributor Author

@gibfahn We don't print the actual commands for many of the other targets. Even we don't print that for cpplint. As it is, I am getting the following in master

EXIT_STATUS=0 ; \
	/Library/Developer/CommandLineTools/usr/bin/make jslint || EXIT_STATUS=$? ; \
	/Library/Developer/CommandLineTools/usr/bin/make cpplint || EXIT_STATUS=$? ; \
	exit $EXIT_STATUS
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \
	  benchmark lib test tools
Total errors found: 0

The first line is definitely not necessary, as it is simply printing the target commands.

@aqrln
Copy link
Contributor

aqrln commented Apr 15, 2017

@thefourtheye yes, the first line should definitely go away from the output :) Not sure about the actual commands for jslint and cpplint, though; I used to copy the eslint command too before it has carved to my memory well enough to be entered immediately without any extra mental overhead.

Even we don't print that for cpplint.

Hmm, I had never pay attention for it, but it is indeed inconsistent. Moreover, this output is somewhat confusing since the command comes from the jslint rule and the output from the cpplint one:

./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \
	  benchmark lib test tools
Total errors found: 0

@refack
Copy link
Contributor

refack commented Apr 15, 2017

what @gibfahn said (I don't know much about make but maybe you have a verbose option, maybe --debug=j)
Also what about vcbuild.bat

@gibfahn
Copy link
Member

gibfahn commented Apr 15, 2017

@gibfahn We don't print the actual commands for many of the other targets.

I think that for each distinct thing we do, we should print something to show we entered that section. Ideally you'd get a single line that you could copy to manually run that section. Then you'd get no other output unless there were errors/warnings. Consider someone running make test, that does about six things. For those of us who've spent hours poring over test logs it's usually pretty straightforward to work out where each output line comes from, but for a new Contributor it's confusing.

The aim is to balance not printing excessive info with not being too opaque IMO. I think tools/test.py and ninja -C do this really well, you get a continuously updating line so you know where you are (but no output is left behind), and then if there's an error you get the line it was on when it errored plus that error. I suspect that might be a little difficult for the Makefile though.

Even we don't print that for cpplint.

Then I think we should print the equivalent for cpplint (unless it's massively long).

The first line is definitely not necessary, as it is simply printing the target commands.

Agreed.

The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.
@thefourtheye
Copy link
Contributor Author

@gibfahn I changed it to print the linter action being carried out. Now the result looks like this

node git:(make-linter-silent) make lint
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \
	  benchmark lib test tools
Running C++ linter...
Total errors found: 0

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Thanks!

jasnell pushed a commit that referenced this pull request Apr 17, 2017
The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.

PR-URL: #12423
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 17, 2017

Landed in 716d831

@jasnell jasnell closed this Apr 17, 2017
@thefourtheye thefourtheye deleted the make-linter-silent branch April 18, 2017 01:55
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.

PR-URL: #12423
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.

PR-URL: #12423
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.

PR-URL: #12423
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request May 16, 2017
The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.

PR-URL: #12423
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.

PR-URL: #12423
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
The linter targets are printing the commands they execute on screen.
This patch reduces the noise by not printing the commands.

PR-URL: nodejs/node#12423
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[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
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants