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,win: goto lint only after defining node_exe #29616

Closed

Conversation

joaocgreis
Copy link
Member

When running vcbuild lint on a new command prompt where vcbuild was not run before, %node_exe% was not defined and eslint.js, lint-js.js and lint-md.js would be run directly using the program defined in Windows (usually the Windows Scripting Host or Notepad).

This moves the goto statement to after %node_exe% is defined.

Fixes: #29602

cc @nodejs/platform-windows

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

When running `vcbuild lint` on a new prompt where vcbuild was not run
before, `%node_exe%` was not defined and `eslint.js`, `lint-js.js`
and `lint-md.js` would be run directly using the program defined in
Windows (usually the Windows Scripting Host or Notepad).

This moves the goto statement to after `%node_exe%` is defined.

Fixes: nodejs#29602
@joaocgreis joaocgreis added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Sep 19, 2019
@joaocgreis
Copy link
Member Author

@bzmlxos did you compile Node by running just vcbuild?

Running the C++ requires having either WSL installed or a copy of GNU Make in the path.

@joaocgreis
Copy link
Member Author

Using GNU Make is a workaround we have in place (ref #28102), a better solution would use Python to run the same code on all platforms. BUILDING.md never got updated, so a PR would probably be welcome.

choco install make is a nice way to get GNU Make, it works for me after two find errors that are unrelated to linting C++.

@bzmlxos the error you see is because the Unix tools that Git installs are not on the PATH. You need to either:

  • re-install git with that option (if using Chocolatey run
    choco install git -params /GitAndUnixToolsOnPath); or
  • use a shell that has the tools on the PATH. One simple way is to open Git Bash and run start. Or you can use Git Bash directly, winpty ./vcbuild.bat lint should work.

@nodejs-github-bot
Copy link
Collaborator

@joaocgreis
Copy link
Member Author

@nodejs/platform-windows @nodejs/build-files this is ready for review, PTAL.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 24, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 27, 2019

Landed in 35bfe0e

@Trott Trott closed this Sep 27, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Sep 27, 2019
When running `vcbuild lint` on a new prompt where vcbuild was not run
before, `%node_exe%` was not defined and `eslint.js`, `lint-js.js`
and `lint-md.js` would be run directly using the program defined in
Windows (usually the Windows Scripting Host or Notepad).

This moves the goto statement to after `%node_exe%` is defined.

Fixes: nodejs#29602

PR-URL: nodejs#29616
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Oct 1, 2019
When running `vcbuild lint` on a new prompt where vcbuild was not run
before, `%node_exe%` was not defined and `eslint.js`, `lint-js.js`
and `lint-md.js` would be run directly using the program defined in
Windows (usually the Windows Scripting Host or Notepad).

This moves the goto statement to after `%node_exe%` is defined.

Fixes: #29602

PR-URL: #29616
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

4 participants