Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
suppress the output of rd %config%#51437
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
liudonghua123 commented Jan 11, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
marco-ippolito commented Jan 11, 2024
cc @nodejs/build-files |
tniessen commented Jan 11, 2024
cc @nodejs/platform-windows |
nodejs-github-bot commented Jan 12, 2024
Uh oh!
There was an error while loading. Please reload this page.
H4ad left a comment
There was a problem hiding this 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 commented Jan 12, 2024
liudonghua123 commented Jan 13, 2024
@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. |
vcbuild.bat Outdated
| :: Suppress "The system cannot find the file specified." on every clean build. | ||
| rd%config%>nul2>nul |
There was a problem hiding this comment.
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?
| :: Suppress "The system cannot find the file specified." on every clean build. | |
| rd%config%>nul2>nul | |
| if exists %config%rd%config% |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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! 😄
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Jan 14, 2024
62c3e0a to 79efff7Comparenodejs-github-bot commented Jan 14, 2024
tniessen commented Jan 14, 2024
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 commented Jan 15, 2024
@tniessen so what should I do? Amend the two commits into one, change the commit message then force-push? |
tniessen commented Jan 15, 2024
That option would be the easiest solution for us, then we only need to re-run CI on the squashed commit :) |
79efff7 to 2f328c0Compareliudonghua123 commented Jan 15, 2024
@tniessen Thanks, now I reset the last two commits and force pushed the amended log message commit. |
tniessen commented Jan 15, 2024
Thank you @liudonghua123! |
nodejs-github-bot commented Jan 15, 2024
nodejs-github-bot commented Jan 16, 2024
nodejs-github-bot commented Jan 21, 2024
nodejs-github-bot commented Jan 25, 2024
Landed in b9a4279 |
PR-URL: nodejs#51437 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51437 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #51437 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51437 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #51437 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #51437 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
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).