build: update lint-cpp on Windows #18012

kfarnung · 2018-01-06T02:24:40Z

Added a lint-cpp argument
Updated findstr calls to output to nul
Updated findstr calls to only use /r when the input is a regex

Checklist

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

Affected core subsystem(s)

build

* Added a `lint-cpp` argument * Updated `findstr` calls to output to `nul` * Updated `findstr` calls to only use `/r` when the input is a regex

jasnell · 2018-01-09T23:31:41Z

ping @nodejs/build

gibfahn · 2018-01-10T08:44:39Z

cc/ @nodejs/platform-windows

joaocgreis · 2018-01-11T16:28:01Z

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/12502/

gibfahn · 2018-01-11T16:48:41Z

vcbuild.bat

 if %errorlevel% equ 0 goto exit

-echo %1 | findstr /r /c:"test\\addons\\[0-9].*_.*\.cc"
+echo %1 | findstr /r /c:"test\\addons\\[0-9].*_.*\.cc" > nul 2>&1


AIUI if one of these files is found then vcbuild bails out, wouldn't it be helpful to see which one it was?

Not the whole vcbuild, only the add-to-list subroutine. I don't really have a strong opinion about the redirects here, stdout is already discarded when calling add-to-list, but it can perhaps help if there's an error in findstr. @kfarnung the output with echo on is the same, is this helpful for you?

@gibfahn This section of code is called elsewhere in the script. Because of this it's treated as though it's a separate batch execution. The early exits here are more akin to early returns in other languages. This merely filters the list of files that are sent into the CPP linter.

@joaocgreis The output is actually to stdout from what I can see. If you test out findstr manually it outputs all successful matches and outputs nothing when it doesn't match. Since echo is being used to pipe into findstr I don't believe that echo on has any effect here.

The reason for the redirects is that I actually found these counter-intuitive. I always assumed it was listing the files it was scanning, not the ones that it was ignoring. If there's strong opposition it might be worth trying to update the output so that it indicates that these are the files being ignored, not the ones being scanned.

joaocgreis · 2018-01-16T12:08:36Z

@gibfahn I can land this, any objection?

gibfahn · 2018-01-16T12:32:07Z

@gibfahn I can land this, any objection?

Not at all, please do!

If there's strong opposition it might be worth trying to update the output so that it indicates that these are the files being ignored, not the ones being scanned.

A comment that explains what the add-to-list subroutine does might be helpful.

Separate topic, but is there a reason to prefer rem over :: in Batch scripts? I find that :: stands out more visually.

gibfahn

My question was answered. LGTM.

richardlau · 2018-01-16T17:49:00Z

Separate topic, but is there a reason to prefer rem over :: in Batch scripts? I find that :: stands out more visually.

Well rem is the officially documented way of commenting in batch files while :: is a hack that abuses the fact that current batch file processing skips over invalid labels. :: also has issues in things like for blocks: https://stackoverflow.com/questions/12407800/which-comment-style-should-i-use-in-batch-files so I'd prefer keeping to rem for consistency.

kfarnung · 2018-01-16T19:17:50Z

@gibfahn I added a comment to the subroutine. Let me know if it's not clear enough.

gibfahn · 2018-01-16T21:54:01Z

@gibfahn I added a comment to the subroutine. Let me know if it's not clear enough.

LGTM

kfarnung · 2018-01-16T22:42:32Z

Running another CI before landing: https://ci.nodejs.org/job/node-test-pull-request/12565/ - PASSED on re-runs except for the Windows failure called out below.

kfarnung · 2018-01-16T23:19:43Z

LinuxOne re-run: https://ci.nodejs.org/job/node-test-commit-linuxone/11913/ - PASSED

kfarnung · 2018-01-16T23:30:48Z

Windows re-run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/14931/

Still seeing a failure, but it looks the same as #18190:

not ok 498 sequential/test-async-wrap-getasyncid
  ---
  duration_ms: 0.284
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 1, actual 2.
        at Object.exports.mustCall (c:\workspace\node-test-binary-windows\test\common\index.js:497:10)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\sequential\test-async-wrap-getasyncid.js:19:35)
        at Module._compile (module.js:660:30)
        at Object.Module._extensions..js (module.js:671:10)
        at Module.load (module.js:577:32)
        at tryModuleLoad (module.js:517:12)
        at Function.Module._load (module.js:509:3)
        at Function.Module.runMain (module.js:701:10)
        at startup (bootstrap_node.js:194:16)
  ...

kfarnung · 2018-01-16T23:46:10Z

Arm re-run: https://ci.nodejs.org/job/node-test-commit-arm/13135/ - PASSED

kfarnung · 2018-01-17T01:26:31Z

Landed in cd60c7d

* Added a `lint-cpp` argument * Updated `findstr` calls to output to `nul` * Updated `findstr` calls to only use `/r` when the input is a regex PR-URL: #18012 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

MylesBorins · 2018-02-27T05:44:52Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

build: update lint-cpp on Windows

58c13eb

* Added a `lint-cpp` argument * Updated `findstr` calls to output to `nul` * Updated `findstr` calls to only use `/r` when the input is a regex

nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jan 6, 2018

kfarnung requested a review from refack January 6, 2018 02:24

joaocgreis approved these changes Jan 11, 2018

View reviewed changes

gibfahn reviewed Jan 11, 2018

View reviewed changes

gibfahn approved these changes Jan 16, 2018

View reviewed changes

[squash] Add comment to subroutine

2a56d41

kfarnung closed this Jan 17, 2018

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added backport-requested-v6.x labels Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: update lint-cpp on Windows #18012

build: update lint-cpp on Windows #18012

kfarnung commented Jan 6, 2018

jasnell commented Jan 9, 2018

gibfahn commented Jan 10, 2018

joaocgreis commented Jan 11, 2018

gibfahn Jan 11, 2018

joaocgreis Jan 11, 2018

kfarnung Jan 11, 2018 •

edited

Loading

joaocgreis commented Jan 16, 2018

gibfahn commented Jan 16, 2018

gibfahn left a comment

richardlau commented Jan 16, 2018

kfarnung commented Jan 16, 2018

gibfahn commented Jan 16, 2018

kfarnung commented Jan 16, 2018 •

edited

Loading

kfarnung commented Jan 16, 2018 •

edited

Loading

kfarnung commented Jan 16, 2018 •

edited

Loading

kfarnung commented Jan 16, 2018 •

edited

Loading

kfarnung commented Jan 17, 2018

MylesBorins commented Feb 27, 2018

build: update lint-cpp on Windows #18012

build: update lint-cpp on Windows #18012

Conversation

kfarnung commented Jan 6, 2018

Checklist

Affected core subsystem(s)

jasnell commented Jan 9, 2018

gibfahn commented Jan 10, 2018

joaocgreis commented Jan 11, 2018

gibfahn Jan 11, 2018

Choose a reason for hiding this comment

joaocgreis Jan 11, 2018

Choose a reason for hiding this comment

kfarnung Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

joaocgreis commented Jan 16, 2018

gibfahn commented Jan 16, 2018

gibfahn left a comment

Choose a reason for hiding this comment

richardlau commented Jan 16, 2018

kfarnung commented Jan 16, 2018

gibfahn commented Jan 16, 2018

kfarnung commented Jan 16, 2018 • edited Loading

kfarnung commented Jan 16, 2018 • edited Loading

kfarnung commented Jan 16, 2018 • edited Loading

kfarnung commented Jan 16, 2018 • edited Loading

kfarnung commented Jan 17, 2018

MylesBorins commented Feb 27, 2018

kfarnung Jan 11, 2018 •

edited

Loading

kfarnung commented Jan 16, 2018 •

edited

Loading

kfarnung commented Jan 16, 2018 •

edited

Loading

kfarnung commented Jan 16, 2018 •

edited

Loading

kfarnung commented Jan 16, 2018 •

edited

Loading