-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
tools: yaml lint command in vcbuild script #48422
base: main
Are you sure you want to change the base?
Conversation
e6bd7fb
to
c196999
Compare
I might be misunderstanding what's going on here, but I think that file will likely be re-added the next time |
Thank you for your response :) I'll try to reconstruct my steps:
I wanted to bring this up before making any changes because I have no evidence this could work on other OSs where I suppose everything works great already. Thank you again for your time and consideration! |
This could use some more reviews. @nodejs/linting @nodejs/platform-windows |
This file is a symbolic link. On Windows, symlinks are disabled by default and must be enabled manually. |
c196999
to
6785e44
Compare
Sorry for the late reply. |
6785e44
to
493af9d
Compare
This needs a rebase. |
Yaml linting was missing for windows users. Reversed evaluation order for wsl and GNUMake of lint-yaml and lint-cpp. Makefile not fully compatible with Windows, so WSL is now preferred. Added "lint-yaml" command reference into "help" command. Added "lint-yaml" step to "test" and "lint" commands.
vcbuild lint-js command would always fail on Windows. eslint.js would inspect the deleted file and throw Syntax Error. Since it didn't seem a meaningful file, it got deleted.
This reverts commit 4dbc523d0027b4918fa292438d7d0ce5dc387f93.
47ff45e
to
7ec56bb
Compare
Done, thank you for the reminder! I've been away from my Windows setup for a bit. |
Hello everyone,
I'm opening this PR hoping it could be an enhancement for Windows contributors.
To give this a little context, when I was working on #48184 , I noticed that
lint-yaml
was run by a Github Action, but it was not present in thevcbuild.bat
script that I used for building and linting files.Following things were changed:
lint-yaml
was added as a command in vcbuild script and it is executed through the Makefile corresponding command.lint-yaml
step was also added and enabled for execution ontest
andlint
commands.wsl make
preferred for executing Makefile command onlint-cpp
andlint-yaml
because of Square brackets not being recognizable by Windows as a command.lint-yaml
reference to thehelp
command.eslint
text file in order to makelint-js
command work again.lint-js
execution would look at that file, throw a Syntax Error thatonFatalError
ineslint.js
would catch and make the linter execution stop.new-lines
type checking from.yamllint.yaml
in order to make linter work on Windows even though the execution was started on a Unix system (such as WSL). I think this could be re-enabled if we are able to use GNU Make instead of WSL to lint those files.I also have two questions:
lint
command would skip the build process if it's already built, is this a behaviour that could be replicated for eachlint-X
command?WSL make
and get rid of the GNU Make option?Thanks for your attention!
@nodejs/build