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

suppress the output of rd %config% #51437

Conversation

liudonghua123
Copy link
Contributor

@liudonghua123 liudonghua123 commented Jan 11, 2024

I noticed the confusing error like message The system cannot find the file specified. in the CIs and on my first build locally. It's really annoying.

And after some debugging, I found the root case of this error message.

Then this simple pr comes for it!

See also #40749 (comment).

@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 Jan 11, 2024
@marco-ippolito
Copy link
Member

cc @nodejs/build-files

@tniessen
Copy link
Member

cc @nodejs/platform-windows

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2024
@anonrig anonrig requested review from H4ad and targos January 12, 2024 01:07
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2024
@nodejs-github-bot
Copy link
Collaborator

vcbuild.bat Outdated Show resolved Hide resolved
@H4ad H4ad added the review wanted PRs that need reviews. label Jan 12, 2024
Copy link
Member

@H4ad H4ad left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@nodejs-github-bot
Copy link
Collaborator

@liudonghua123
Copy link
Contributor Author

@H4ad Hi, could we merge the pull request.

I noticed some warnings/errors as follows in https://ci.nodejs.org/job/node-test-commit/67906/console, but I couldn't help to fix it.

20:26:32 git@github.com: Permission denied (publickey).
20:26:32 fatal: Could not read from remote repository.

vcbuild.bat Outdated
Comment on lines 340 to 341
:: Suppress "The system cannot find the file specified." on every clean build.
rd %config% > nul 2> nul
Copy link
Member

Choose a reason for hiding this comment

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

Would something like this work instead to not suppress other errors?

Suggested change
:: Suppress "The system cannot find the file specified." on every clean build.
rd %config% > nul 2> nul
if exists %config% rd %config%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct syntax should be
"IF EXIST file command". However, check whether the file exists doesn't help a lot here because it's safe to remove the link even the link doesn't exists. Both of them works, and RD directly is just more simpler IMO.

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, if %config% doesn't exist, it's not necessary to invoke rd, and when it does exist, this approach doesn't silently ignore the output of rd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I updated this pr. Thanks! 😄

vcbuild.bat Outdated Show resolved Hide resolved
@H4ad H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@liudonghua123 liudonghua123 force-pushed the fix_rd_confusing_error_like_message branch from 62c3e0a to 79efff7 Compare January 14, 2024 12:50
@H4ad H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member

Note @ whoever decides to merge this: the commit queue would incorrectly use the first commit message as the final commit message, so either land this manually or force-push to the PR branch with an amended commit message (but that requires a fresh CI).

@liudonghua123
Copy link
Contributor Author

Note @ whoever decides to merge this: the commit queue would incorrectly use the first commit message as the final commit message, so either land this manually or force-push to the PR branch with an amended commit message (but that requires a fresh CI).

@tniessen so what should I do? Amend the two commits into one, change the commit message then force-push?

@tniessen
Copy link
Member

That option would be the easiest solution for us, then we only need to re-run CI on the squashed commit :)

@liudonghua123 liudonghua123 force-pushed the fix_rd_confusing_error_like_message branch from 79efff7 to 2f328c0 Compare January 15, 2024 12:45
@liudonghua123
Copy link
Contributor Author

@tniessen Thanks, now I reset the last two commits and force pushed the amended log message commit.

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2024
@tniessen
Copy link
Member

Thank you @liudonghua123!

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2024
@nodejs-github-bot nodejs-github-bot merged commit b9a4279 into nodejs:main Jan 25, 2024
51 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b9a4279

@liudonghua123 liudonghua123 deleted the fix_rd_confusing_error_like_message branch January 26, 2024 00:08
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 2, 2024
PR-URL: nodejs#51437
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Feb 9, 2024
PR-URL: nodejs#51437
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Feb 15, 2024
PR-URL: #51437
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024
PR-URL: nodejs#51437
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51437
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51437
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
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. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants