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: update lint-cpp on Windows #18012

Closed
wants to merge 2 commits into from
Closed

Conversation

kfarnung
Copy link
Contributor

@kfarnung kfarnung commented Jan 6, 2018

  • 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
@nodejs-github-bot 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 kfarnung requested a review from refack January 6, 2018 02:24
@jasnell
Copy link
Member

jasnell commented Jan 9, 2018

ping @nodejs/build

@gibfahn
Copy link
Member

gibfahn commented Jan 10, 2018

cc/ @nodejs/platform-windows

@joaocgreis
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@kfarnung kfarnung Jan 11, 2018

Choose a reason for hiding this comment

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

@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
Copy link
Member

@gibfahn I can land this, any objection?

@gibfahn
Copy link
Member

gibfahn commented Jan 16, 2018

@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.

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.

My question was answered. LGTM.

@richardlau
Copy link
Member

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
Copy link
Contributor Author

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

@gibfahn
Copy link
Member

gibfahn commented Jan 16, 2018

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

LGTM

@kfarnung
Copy link
Contributor Author

kfarnung commented Jan 16, 2018

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
Copy link
Contributor Author

kfarnung commented Jan 16, 2018

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

@kfarnung
Copy link
Contributor Author

kfarnung commented Jan 16, 2018

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
Copy link
Contributor Author

kfarnung commented Jan 16, 2018

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

@kfarnung
Copy link
Contributor Author

Landed in cd60c7d

@kfarnung kfarnung closed this Jan 17, 2018
kfarnung added a commit that referenced this pull request Jan 17, 2018
* 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>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
* 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 pushed a commit that referenced this pull request Feb 27, 2018
* 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
Copy link
Contributor

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.

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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants