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: support lint-js-fix in vcbuild.bat #53046

Closed
wants to merge 1 commit into from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented May 18, 2024

This PR adds support for lint-js-fix in the vcbuild.bat file.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. windows Issues and PRs related to the Windows platform. labels May 18, 2024
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 19, 2024
@anonrig
Copy link
Member

anonrig commented May 19, 2024

cc @nodejs/platform-windows

@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 19, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 19, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@huseyinacacak-janea
Copy link
Contributor

Hey @RedYetiDev, the labels you created should be called from other command in order to be executed. Feel free to use the patch I'm sending.

Another thing, this is not as important, I think lint-md-fix and lint-js-fix should go before lint-md and lint-js, just in case someone tries to run both commands at the same time.

From e45abb7e84c32d7403a196303e406ed775580261 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?H=C3=BCseyin=20A=C3=A7acak?= <huseyin@janeasystems.com>
Date: Mon, 20 May 2024 14:24:40 +0300
Subject: [PATCH 1/1] build: fix lint-md-fix and lint-js-fix

---
 vcbuild.bat | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/vcbuild.bat b/vcbuild.bat
index f07711fab7..1b71db935d 100644
--- a/vcbuild.bat
+++ b/vcbuild.bat
@@ -707,11 +707,11 @@ goto lint-js
 goto lint-js
 
 :lint-js
-if not defined lint_js goto lint-md-build
+if not defined lint_js goto lint-js-fix
 if not exist tools\node_modules\eslint goto no-lint
 echo running lint-js
 %node_exe% tools\node_modules\eslint\bin\eslint.js --cache --max-warnings=0 --report-unused-disable-directives --rule "linebreak-style: 0" .eslintrc.js benchmark doc lib test tools
-goto lint-md-build
+goto lint-js-fix
 
 :lint-js-fix
 if not defined lint_js_fix goto lint-md-build
@@ -731,7 +731,7 @@ echo "Deprecated no-op target 'lint_md_build'"
 goto lint-md
 
 :lint-md
-if not defined lint_md goto format-md
+if not defined lint_md goto lint-md-fix
 echo Running Markdown linter on docs...
 SETLOCAL ENABLEDELAYEDEXPANSION
 set lint_md_files=
@@ -742,7 +742,7 @@ for /D %%D IN (doc\*) do (
 )
 %node_exe% tools\lint-md\lint-md.mjs %lint_md_files%
 ENDLOCAL
-goto format-md
+goto lint-md-fix
 
 :lint-md-fix
 if not defined lint_md_fix goto format-md
-- 
2.43.0.windows.1

@RedYetiDev
Copy link
Member Author

I disagree, there are times that people want to lint without fixes

@RedYetiDev
Copy link
Member Author

Another thing, this is not as important, I think lint-md-fix and lint-js-fix should go before lint-md and lint-js, just in case someone tries to run both commands at the same time.

That won't happen, as there can only be one %1 argument (IIUC)

@huseyinacacak-janea
Copy link
Contributor

That won't happen, as there can only be one %1 argument (IIUC)

Theoretically, if you call vcbuild.bat lint-js lint-js-fix it will do both. The value of %1 is changed with the usage of shift command here. As I said, this part is a suggestion.

The patch file I provided enables the 2 new commands. Running vcbuild.bat lint-js-fix does nothing for me locally unless the patch is applied. I do not see running lint-js-fix which should be there.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 20, 2024

That won't happen, as there can only be one %1 argument (IIUC)

Theoretically, if you call vcbuild.bat lint-js lint-js-fix it will do both. The value of %1 is changed with the usage of shift command here. As I said, this part is a suggestion.

The patch file I provided enables the 2 new commands. Running vcbuild.bat lint-js-fix does nothing for me locally unless the patch is applied. I do not see running lint-js-fix which should be there.

Good to know, thanks (I'm not a batch file expert)

@RedYetiDev
Copy link
Member Author

I've made your suggestions. Sorry I brushed your comment off initially, I'm not a batch file expert and I didn't quite understand how goto worked.

@huseyinacacak-janea
Copy link
Contributor

No problem, glad I could help.

@RedYetiDev RedYetiDev changed the title build: support lint-md-fix and lint-js-fix in vcbuild.bat build: support lint-js-fix in vcbuild.bat May 21, 2024
@RedYetiDev
Copy link
Member Author

To summarize, I darn goofed and --fix isn't supported by the markdown linter.

@aduh95
Copy link
Contributor

aduh95 commented May 23, 2024

Could someone with a Windows machine validate that this works so this can land?

@H4ad
Copy link
Member

H4ad commented May 23, 2024

The script is being called correctly, so this PR is good.


But I'm getting an error (which is not related to this PR) that is:

$ node.exe tools\node_modules\eslint\bin\eslint.js --cache --max-warnings=0 --report-unused-disable-directives --rule "linebreak-style: 0" .eslintrc.js benchmark doc lib test tools

Oops! Something went wrong! :(

ESLint: 8.57.0

C:\Users\vinic\Desktop\Projects\node\tools\node_modules\eslint\node_modules\eslint:1
..
^

SyntaxError: Unexpected token '.'
    at internalCompileFunction (node:internal/vm:128:18)
    at wrapSafe (node:internal/modules/cjs/loader:1279:20)
    at Module._compile (node:internal/modules/cjs/loader:1331:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1426:10)
    at Module.load (node:internal/modules/cjs/loader:1205:32)
    at Module._load (node:internal/modules/cjs/loader:1021:12)
    at Module.require (node:internal/modules/cjs/loader:1230:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (C:\Users\vinic\Desktop\Projects\node\tools\node_modules\eslint\node_modules\eslint-plugin-jsdoc\dist\rules\checkExamples.cjs:8:39)
    at Module._compile (node:internal/modules/cjs/loader:1368:14)

Any recommendations on what to do here?

@huseyinacacak-janea
Copy link
Contributor

@H4ad eslint is a symlink that doesn't work by default in Git. To fix this issue, the repo should be cloned as follows
git clone -c core.symlinks=true <repository_url>

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 23, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 23, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/53046
✔  Done loading data for nodejs/node/pull/53046
----------------------------------- PR info ------------------------------------
Title      build: support `lint-js-fix` in `vcbuild.bat` (#53046)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     RedYetiDev:patch-32 -> nodejs:main
Labels     windows, build, author ready, needs-ci
Commits    4
 - build: support `lint-md-fix` and `lint-js-fix` in `vcbuild.bat`
 - Update vcbuild.bat
 - Update vcbuild.bat
 - Update vcbuild.bat
Committers 1
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/53046
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Moshe Atlow 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53046
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Moshe Atlow 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 18 May 2024 13:18:20 GMT
   ✔  Approvals: 3
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/53046#pullrequestreview-2064964306
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/53046#pullrequestreview-2065008304
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/53046#pullrequestreview-2073766737
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-05-19T07:10:15Z: https://ci.nodejs.org/job/node-test-pull-request/59298/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - Update vcbuild.bat
   ⚠  - Update vcbuild.bat
- Querying data for job/node-test-pull-request/59298/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/9208503129

@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2024
@nodejs-github-bot
Copy link
Collaborator

@H4ad
Copy link
Member

H4ad commented May 25, 2024

Nothing more sad than a green CI and a conflict, @RedYetiDev can you rebase?

@RedYetiDev
Copy link
Member Author

Fingers crossed nothing broke

@nodejs-github-bot
Copy link
Collaborator

@H4ad H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2024
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented May 25, 2024

@RedYetiDev can you please rebase to get rid of the merge commit that the CI is choking on?

@RedYetiDev RedYetiDev force-pushed the patch-32 branch 2 times, most recently from f8abafc to 72cf866 Compare May 25, 2024 19:50
@RedYetiDev
Copy link
Member Author

😌 Rebase complete

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Jun 30, 2024

This needs another rebase.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 2, 2024

@aduh95 done :-)

@RedYetiDev
Copy link
Member Author

The merge conflicts won't go away :/, I might need to open a new PR.

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. needs-ci PRs that need a full CI run. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants