[v6.x backport] build: enable cpplint on windows #14879
Closed
Conversation
This file contains 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
Also fix repl and url libs for the rule. PR-URL: nodejs#14032 Refs: http://eslint.org/docs/rules/no-use-before-define Reviewed-By: Daijiro Wachi <[email protected]>
Because of a race condition, connection listener may not be invoked if test is run under load. Remove `common.mustCall()` wrapper from the listener. Move the test to `parallel` because it now works under load. Make similar change to http test to keep them in synch even though it is much harder to trigger the race in http. PR-URL: nodejs#14134 Fixes: nodejs#14133 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
The tests include a callback that might not be invoked but is wrapped in common.mustCall(). Remove the common.mustCall() wrapper and add a comment explaining that it should not be added. Add a new test case that sets the timeout to 1ms and waits for both the connection handler and the timeout handler to be invoked. This version keeps the common.mustCall() wrapper intact around the connection handler (although it's mostly semantic and not necessary for the test as the test will certainly fail or time out if that handler isn't invoked). PR-URL: nodejs#14380 Fixes: nodejs#11768 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#14343 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#14343 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Do not repeat RegExp creation in cycle. * Use sufficient string instead of RegExp in split(). PR-URL: nodejs#13709 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Update ESLint to 4.1.0. This fixes a bug that previously prevented us from using the new and stricter indentation checking. Refs: eslint/eslint#8721 Backport-PR-URL: nodejs#14830 PR-URL: nodejs#13895 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Provide a bash script for updating ESLint in the project. Backport-PR-URL: nodejs#14830 PR-URL: nodejs#13895 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#13946 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: James M Snell <[email protected]>
If the filesystem does not support UCS2, do not run the test. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#14029 Fixes: nodejs#14028 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]>
In preparation for stricter indentation linting, remove the align-multiline-assignment custom rule, as it may conflict with the ESLint stricter indentation linting. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#14079 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
In anticipation of stricter linting for indentation issues, modify ternary operators in lib that do not conform with the expected ESLint settings. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#14078 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
In anticipation of stricter linting for indentation, remove instances of extra indentation that will be flagged by the new rules. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#14090 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
In anticipation of stricter indentation linting, normalize indentation of code in parentheses. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#14125 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
In preparation for stricter indentation linting and to increase code clarity, update indentation for ternaries in lib. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#14247 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
ESLint 4.x has stricter linting than previous versions. We are currently using the legacy indentation rules in the test directory. This commit changes the indentation of files to comply with the stricter 4.x linting and enable stricter linting in the test directory. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#14431 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
In preparation for stricter ESLint indentation checking, fix a few issues in sample code. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#13950 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
All linting now uses the current ESLint 4.3.0 indentation linting. Remove legacy indentation rules. Backport-PR-URL: nodejs#14835 PR-URL: nodejs#14515 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
As indicated by the FIXME comment, this macro guard is no longer needed. PR-URL: nodejs#12638 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
* Make common.skip() exit. Also add common.printSkipMessage() for partial skips. * Don't make needless things before skip Backport-PR-URL: nodejs#14838 PR-URL: nodejs#14021 Fixes: nodejs#14016 Reviewed-By: Refael Ackermann <[email protected]>
Backport-PR-URL: nodejs#14842 PR-URL: nodejs#13900 Fixes: nodejs#13882 Reviewed-By: Tobias Nießen <[email protected]>
* rename :exit to :distexit Backport-PR-URL: nodejs#14842 PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
Handle spaces in the path to python.exe, in case it is installed under some directory like "C:\Program Files". Backport-PR-URL: nodejs#14842 PR-URL: nodejs#14546 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Instead of generating string concatenation, generate a template literal. This is mostly useful as a pre-emptive measure for avoiding problems when (if?) we enable the prefer-template lint rule in the test directory. PR-URL: nodejs#14094 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Explain the behavior of `fs.open()` under win32 that file path contains some characters and add some test cases for them. < (less than) > (greater than) : (colon) " (double quote) / (forward slash) \ (backslash) | (vertical bar or pipe) ? (question mark) * (asterisk) PR-URL: nodejs#13875 Refs: nodejs#13868 Refs: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx Refs: https://msdn.microsoft.com/en-us/library/windows/desktop/bb540537.aspx Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
If the first parameter of `request.end` `data` is specified, it should be equivalent to calling `request.write(data, encoding)` (not `response.write(data, encoding)`) followed by `request.end(callback)`. This mistake was introduced in commit 14b3aab: date: 28 November 2015 at 7:30:32 AM GMT+8 author: jpersson <[email protected]> committer: James M Snell <[email protected]> summary: doc: add links and backticks around names PR-URL: nodejs#14126 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: David Cai <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
PR-URL: nodejs#13576 Fixes: nodejs#13197 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
If allowHalfOpen is set to false, the stream will automatically end the writable side when the readable side ends, but not the other way around. PR-URL: nodejs#14127 Fixes: nodejs#4044 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
3c92ca2 should have had tests to go along with it. This adds tests for the following functions: * `process.geteuid()` * `process.seteuid()` * `process.getegid()` * `process.setegid()` PR-URL: nodejs#14091 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
This change removes `common.noop` from the Node.js internal testing
common module.
Over the last few weeks, I've grown to dislike the `common.noop`
abstraction.
First, new (and experienced) contributors are unaware of it and so it
results in a large number of low-value nits on PRs. It also increases
the number of things newcomers and infrequent contributors have to be
aware of to be effective on the project.
Second, it is confusing. Is it a singleton/property or a getter? Which
should be expected? This can lead to subtle and hard-to-find bugs. (To
my knowledge, none have landed on master. But I also think it's only a
matter of time.)
Third, the abstraction is low-value in my opinion. What does it really
get us? A case could me made that it is without value at all.
Lastly, and this is minor, but the abstraction is wordier than not using
the abstraction. `common.noop` doesn't save anything over `() => {}`.
So, I propose removing it.
PR-URL: nodejs#12822
Backport-PR-URL: nodejs#14174
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
* Update the experimental status to reflect actual common use. * Also make a few formatting fixes. Fixes: nodejs#12701 PR-URL: nodejs#12723 Fixes: nodejs#12701 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
PR-URL: nodejs#14284 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs#14270 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: David Cai <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
test/parallel/test-force-repl.js has an unnecessary timer that makes the test flaky under load. Remove it. PR-URL: nodejs#14439 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
If JSON.parse() fails, print a message showing the JSON that failed to parse. This is to help with debugging a current test failure on CI. PR-URL: nodejs#14508 Ref: nodejs#14507 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
It doesn't seem to make much sense to have the mentioned typedef
declaration equipped with NODE_EXTERN. In fact, when compiling with GCC,
an attribute specifier like __attribute__((visibility("default"))) in
such a typedef declaration will cause the following warning message:
warning: ‘visibility’ attribute ignored [-Wattributes]
The issue goes unnoticed because NODE_EXTERN is defined as nothing for
GCC builds, but for correctness it's better to not specify it here at
all.
PR-URL: nodejs#14466
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#14546 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
This commit calls require on a shared library that is not declared as a node module, and therefore does not register properly. PR-URL: nodejs#13954 Signed-off-by: Ezequiel Garcia <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* block scope test cases * clean up global leaks in individual test cases * enable global variable leak checking * remove console.error() statements PR-URL: nodejs#14536 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
* use common.mustCall() instead of exit handler * use execSync instead of exec so test is reliable under load * move from sequential to parallel PR-URL: nodejs#14541 Fixes: nodejs#11826 Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
This is a very very minor change. PR-URL: nodejs#14587 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs#14692 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#14417 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
This PR adds cpp linting to windows build script. After this change, running command `vcbuild lint` will run both cpp linting and javascript linting on a windows machine. PR-URL: nodejs#11856 Fixes: nodejs#11816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Refactors cpplint slightly to allow multiple runs of it. This allows downstream projects to run cpplint on their dependencies. PR-URL: nodejs#14116 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: João Reis <[email protected]>
nodejs-github-bot
added
build
|
@MylesBorins @refack It turns out that the capability to even run cpplint wasn't originally backported to v6.x so I went ahead and did that as well. |
refack
approved these changes
Aug 22, 2017
PR-URL: nodejs#11992 Fixes: nodejs#11971 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins
force-pushed
the
v6.x-staging
branch
2 times, most recently
from
aaf4e13 to
31f572c
Compare
MylesBorins
pushed a commit
that referenced
this pull request
Sep 19, 2017
This PR adds cpp linting to windows build script. After this change, running command `vcbuild lint` will run both cpp linting and javascript linting on a windows machine. Backport-PR-URL: #14879 PR-URL: #11856 Fixes: #11816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins
pushed a commit
that referenced
this pull request
Sep 19, 2017
Refactors cpplint slightly to allow multiple runs of it. This allows downstream projects to run cpplint on their dependencies. Backport-PR-URL: #14879 PR-URL: #14116 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: João Reis <[email protected]>
|
landed in 38ae5c4...14cc1ab |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I backported two changes which enable cpplint to be run on windows.
Refs: #11856
Refs: #14116
Refs: #11992
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build