ci: [refactor] Drop last use of pwsh#34583
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
eb30585 to
faba7f6
Compare
.github/ci-windows-cross.py
Outdated
| vsdevcmd = Path(installation_path) / "Common7" / "Tools" / "vsdevcmd.bat" | ||
| comspec = os.environ["COMSPEC"] | ||
| output = run( | ||
| f'"{comspec}" /s /c ""{vsdevcmd}" -arch=x64 -no_logo && set"', |
There was a problem hiding this comment.
The double citation marks "" after /c make me uneasy, but I guess the string parsing might disallow empty strings and so can support keeping track of one level of quoted strings internally.
There was a problem hiding this comment.
This is just a literal copy of what the pwsh script did (it also had the exact same full string with the exact same double quotes), which are required for cmd.exe. I'd be curious to see if there is a way (at all) to do this with an array of strings, but maybe this can be done in a follow-up by a Windows person? (I don't have Windows, so testing all of this with CI isn't smooth)
There was a problem hiding this comment.
This is just a literal copy of what the pwsh script did (it also had the exact same full string with the exact same double quotes)
It had some back-ticks in there, but I don't see what difference they would make.
bitcoin/.github/workflows/ci.yml
Line 236 in b65ff0e
There was a problem hiding this comment.
It had some back-ticks in there, but I don't see what difference they would make.
I think the back-ticks are just some pwsh thing to escape the " char, but I am not 100% sure. (This confusion is one of the reasons to drop pwsh)
In python that isn't needed, because '-strings can contain " chars as-is.
I am still curious if a Windows person or a Windows LLM can refactor subprocess.run(f'"{comspec}" /s /c ""{vsdevcmd}" -arch=x64 -no_logo && set")' into the run([...]) form.
|
utACK faba7f6 Wasn't sure which python file the setup vs prompt belonged but agree it's perhaps a bit silly to add another file just for this. I wasn't able to test it on my windows machine as it requires Github environment vars. |
Also, change the yaml anchor name and the step name. Also, small refactors while touching the files.
Seems easier to just use Bash and Python consistently.
faba7f6 to
fa36ade
Compare
.github/ci-windows-cross.py
Outdated
| vsdevcmd = Path(installation_path) / "Common7" / "Tools" / "vsdevcmd.bat" | ||
| comspec = os.environ["COMSPEC"] | ||
| output = run( | ||
| f'"{comspec}" /s /c ""{vsdevcmd}" -arch=x64 -no_logo && set"', |
There was a problem hiding this comment.
This is just a literal copy of what the pwsh script did (it also had the exact same full string with the exact same double quotes)
It had some back-ticks in there, but I don't see what difference they would make.
bitcoin/.github/workflows/ci.yml
Line 236 in b65ff0e
|
Looks good! re-ACK fa36ade |
The use of pwsh was a bit confusing and inconsistent around the exit code. See also #32672
I think it is fine to drop it and purely use Bash/Python.
This also moves a bit of code from the github yaml to the python script.