-
Notifications
You must be signed in to change notification settings - Fork 49
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
Random failure with error "Command failed: cmd.exe /q /c C:\Users\runneradmin\msvc-dev-cmd.bat" #8
Random failure with error "Command failed: cmd.exe /q /c C:\Users\runneradmin\msvc-dev-cmd.bat" #8
Comments
@ilammy Any update on this. Due to this random error, my CI is completely unresponsive and unreliable. |
Hey, @coder3101! I've just pushed a new v1.3.0 with that batch file change. I'm still puzzled at what can break there, but could you please try it out?
|
Nothing was broken rather what was is the behaviour of this extension if a msvc of a version is requested which is not installed? Say 14.25 on windows-2019 image is no longer present. So I changed it to 14.26 and now it works fine. Maybe instead of just error-ing out the extension should say "this version of msvc does not exist" |
Hmm, I am running into this as well. This only happens for me with
This works for arm and x86, then fails to setup amd64.
|
Seems to only fail after the second usage, order doesn't appear to matter. |
I could reproduce this as well.
|
🤔 Does this issue reproduce with jobs:
...:
- uses: ilammy/msvc-dev-cmd@v1.3.0 Or maybe it prints more helpful error message if it fails. Currently |
Could we add a try catch to this line, so we can get more information? Line 95 in 4b3ec49
Also, we should use execSync here. Awaiting a promise right away is equal to synchronous execution. |
If it throws, the line 111 catches it: Line 111 in 4b3ec49
And that's the message being printed. From what I gather from Node.js docs, it should contain more information than just the message, like captured stderr, but I'm not quite sure how to access it.
Makes sense 🤔 |
Hi, @coder3101 .
Good news: the toolset 14.25 has been back on 1 July 2020. See actions/runner-images#1146 . |
Hi, @kwhat .
Thank you for posting the screenshot of the log. Let's focus on these two lines:
AnalysisTry to find them on Internet:
Does the command line exceed the limit of 8192 characters? We can test it on GitHub Actions or our own computer with MSVC installed: runs-on: windows-2019
steps:
- shell: cmd
run: |
:loop
echo ======
call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x64
echo ------
set
echo ======
goto loop Why use In my test, the workflow failed on the third execution of vcvarsall.bat:
As you can see, each execution of vcvarsall.bat adds (6971 - 4822 = 2149) characters to the head of the PATH environment variable. Therefore, during the third execution, after multiple executions of commands like What is
SuggestionYou should not run this action multiple times. If you want to build an application with multiple development environments on GitHub Actions, you should use the |
Wow, @pzhlkj6612, thanks for a detailed analysis! (And the one in the other issue as well). Somehow it did not occur to me that @kwhat's issue might be caused by environment variable overflow. I can still reproduce the issue, but I'm unsure what to do about it. I guess your advice to avoid running this action multiple time is the best. You don't normally launch a developer command prompt out of another developer command prompt, the same is true here. I have played around with deduplicating path-like environment variables, attempting to correct the behavior of I believe it would work to launch this action multiple times in a row, in some limited use cases, but I would certainly not recommend it. I guess I will settle on applying the deduplication hack, coupled with maybe issuing a warning on repeated invocations (with an option to suppress it if you're sure it works for you). I'll think on an approach for that. |
(GitHub should provide a way to opt out of automatically closing issues mentioned in a PR. The fact that something has been merged does not mean that it was released. And the fact that it was released, does not mean that the issue has been resolved.) |
Well, you used the "Resolves" keyword to link this issue with that PR, so it happened. |
@kwhat, I've just released v1.8.0 which should allow reconfiguration. Could you please try it out?
|
Yeah, that thing exists. But it would be nice to be able to still use those words as markers without actually closing issues. There is a list of linked PRs and issues, but it's not possible to “uncheck” things there to prevent automatic closure. </rant> |
Indeed. We can only use "plain" references like |
What if we set the value of those variables to empty? |
Then they will be empty. A variable with "empty string" as a value. This is different from "not present in the environment". How this is handled is up to the software that queries them. It might consider them missing, or it might fail with an "invalid value" error. While I haven't noticed any variables so far that could cause problems, the possibility still remains. I thought that it's better to allow reconfiguration in some way over not allowing it at all. If it works in most cases and no one runs into issues – great. If this does not work for someone – I hope they'll raise an issue and that specific issues can be addressed, rather then me trying to prevent a hypothetical situations proactively by applying random hacks. (I mean, if there was an obvious documented way to remove variables and if |
The action randomly fails with the following log, sometimes
Check this Job which failed: https://github.com/BoostGSoC20/ublas/runs/726808960?check_suite_focus=true
Check this job with the same configuration that passed: https://github.com/BoostGSoC20/ublas/runs/726808920?check_suite_focus=true
This failure is completely random. I looked into it and found that in the latest release this action still uses some script but after 5a6b51d, this behaviour changed. Please look into it and if possible you could release a new version with this commit.
The text was updated successfully, but these errors were encountered: