ci: [refactor] Drop last use of pwsh #34583

I see ci-windows-cross.py is already used for other steps within this native (non-cross) job. Why isn't ci-windows.py used here?

I think this is the first one. Happy to move it over to the other python script. I don't think it matters much. The goal is just to avoid having to add a third python script for this stand-alone github-only snippet

Ah, that's right, the others are "windows-native-test", which is testing the cross executables. I'd rather put it in ci-windows.py for maximum tidyness.

thx. May do, if I have to re-push

Actually, .github/ci-windows.py requires an unused positional arg, which is equally confusing. Will leave as-is for now.

I think using what is arguably the wrong file is more confusing than an unused arg (which could be made optional), but it's not a blocker.

Happy to push any diff, if someone writes one, or push any commit, if someone writes one, but I can't get myself to write it :sweat_smile:

Also, I am happy to review a follow-up doing this.

<details><summary>Possibly too opinionated patch...</summary>

Uses raw symbols for the function list and execution. Attempts to improve naming.

diff --git a/.github/ci-windows-cross.py b/.github/ci-windows-cross.py
index d32233258c..13ca3b4945 100755
--- a/.github/ci-windows-cross.py
+++ b/.github/ci-windows-cross.py
@@ -20,29 +20,6 @@ def run(cmd, **kwargs):
         sys.exit(str(e))
 
 
-def setup_vs_dev_prompt():
-    vswhere_path = Path(os.environ["ProgramFiles(x86)"]) / "Microsoft Visual Studio" / "Installer" / "vswhere.exe"
-    installation_path = run(
-        [str(vswhere_path), "-latest", "-property", "installationPath"],
-        capture_output=True,
-        text=True,
-    ).stdout.strip()
-    vsdevcmd = Path(installation_path) / "Common7" / "Tools" / "vsdevcmd.bat"
-    comspec = os.environ["COMSPEC"]
-    output = run(
-        f'"{comspec}" /s /c ""{vsdevcmd}" -arch=x64 -no_logo && set"',
-        capture_output=True,
-        text=True,
-    ).stdout
-    github_env = os.environ["GITHUB_ENV"]
-    with open(github_env, "a") as env_file:
-        for line in output.splitlines():
-            if "=" not in line:
-                continue
-            name, value = line.split("=", 1)
-            env_file.write(f"{name}={value}\n")
-
-
 def print_version():
     bitcoind = Path.cwd() / "bin" / "bitcoind.exe"
     run([str(bitcoind), "-version"])
@@ -158,7 +135,6 @@ def run_unit_tests():
 def main():
     parser = argparse.ArgumentParser(description="Utility to run Windows CI steps.")
     steps = [
-        "github_env_setup_vs_dev_prompt",
         "print_version",
         "check_manifests",
         "prepare_tests",
@@ -173,9 +149,7 @@ def main():
         str(Path.cwd() / "previous_releases"),
     )
 
-    if args.step == "github_env_setup_vs_dev_prompt":
-        setup_vs_dev_prompt()
-    elif args.step == "print_version":
+    if args.step == "print_version":
         print_version()
     elif args.step == "check_manifests":
         check_manifests()
diff --git a/.github/ci-windows.py b/.github/ci-windows.py
index caa2d52c77..212d0de7c5 100755
--- a/.github/ci-windows.py
+++ b/.github/ci-windows.py
@@ -38,6 +38,29 @@ GENERATE_OPTIONS = {
 }
 
 
+def github_import_vs_env(ci_type):
+    vswhere_path = Path(os.environ["ProgramFiles(x86)"]) / "Microsoft Visual Studio" / "Installer" / "vswhere.exe"
+    installation_path = run(
+        [str(vswhere_path), "-latest", "-property", "installationPath"],
+        capture_output=True,
+        text=True,
+    ).stdout.strip()
+    vsdevcmd = Path(installation_path) / "Common7" / "Tools" / "vsdevcmd.bat"
+    comspec = os.environ["COMSPEC"]
+    output = run(
+        f'"{comspec}" /s /c ""{vsdevcmd}" -arch=x64 -no_logo && set"',
+        capture_output=True,
+        text=True,
+    ).stdout
+    github_env = os.environ["GITHUB_ENV"]
+    with open(github_env, "a") as env_file:
+        for line in output.splitlines():
+            if "=" not in line:
+                continue
+            name, value = line.split("=", 1)
+            env_file.write(f"{name}={value}\n")
+
+
 def generate(ci_type):
     command = [
         "cmake",
@@ -50,7 +73,7 @@ def generate(ci_type):
     run(command)
 
 
-def build():
+def build(ci_type):
     command = [
         "cmake",
         "--build",
@@ -180,26 +203,18 @@ def run_tests(ci_type):
 def main():
     parser = argparse.ArgumentParser(description="Utility to run Windows CI steps.")
     parser.add_argument("ci_type", choices=GENERATE_OPTIONS, help="CI type to run.")
-    steps = [
-        "generate",
-        "build",
-        "check_manifests",
-        "prepare_tests",
-        "run_tests",
-    ]
+    steps = list(map(lambda f: f.__name__, [
+        github_import_vs_env,
+        generate,
+        build,
+        check_manifests,
+        prepare_tests,
+        run_tests,
+    ]))
     parser.add_argument("step", choices=steps, help="CI step to perform.")
     args = parser.parse_args()
 
-    if args.step == "generate":
-        generate(args.ci_type)
-    elif args.step == "build":
-        build()
-    elif args.step == "check_manifests":
-        check_manifests(args.ci_type)
-    elif args.step == "prepare_tests":
-        prepare_tests(args.ci_type)
-    elif args.step == "run_tests":
-        run_tests(args.ci_type)
+    exec(f'{args.step}("{args.ci_type}")')
 
 
 if __name__ == "__main__":
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 4942ac34e8..bc56774e28 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -227,9 +227,9 @@ jobs:
 
       - *CHECKOUT
 
-      - &SET_UP_VS
-        name: Set up VS Developer Prompt
-        run: py -3 .github/ci-windows-cross.py github_env_setup_vs_dev_prompt
+      - &IMPORT_VS_ENV
+        name: Import Visual Studio env vars
+        run: py -3 .github/ci-windows.py ${{ matrix.job-type }} github_import_vs_env
 
       - name: Get tool information
         run: |
@@ -411,7 +411,7 @@ jobs:
       - name: Run bitcoind.exe
         run: py -3 .github/ci-windows-cross.py print_version
 
-      - *SET_UP_VS
+      - *IMPORT_VS_ENV
 
       - name: Check executable manifests
         run: py -3 .github/ci-windows-cross.py check_manifests

</details>

Are you sure the patch applies? It looks corrupt and does not apply locally.

Maybe push a commit, so I can just force push it to this pull?

Seems like I double-tapped paste, sorry. Fixed now, verified it applies to cleanly on top of faba7f616db05ded6cc642821ce540aa40fb8fcb.

I don't think the patch will pass ci as-is, because there is no job-type in the cross test.

So I've pushed it with some modifications.

Thanks for taking the patch and fixing the job-type!

If we actually need this for testing already built cross builds, could it be called something clearer like IMPORT_VS_ENV?

Yes, it is needed. Otherwise:

+ mt.exe -nologo '-inputresource:D:\a\bitcoin\bitcoin\bin\bitcoind.exe' '-out:D:\a\bitcoin\bitcoin\bin\bitcoind.manifest'
[WinError 2] The system cannot find the file specified
Error: Process completed with exit code 1.

https://github.com/bitcoin/bitcoin/actions/runs/22061363427/job/63743877223?pr=34583#step:6:11

Makes sense. Still think "IMPORT_VS_ENV" is clearer than "SET_UP_VS" but it's tangential to this PR.

thx. May do, if I have to re-push

The yaml anchor made more sense when there was a larger template. As it's just a call to python now could the anchor just be dropped entirely?

The yaml anchor made more sense when there was a larger template. As it's just a call to python now could the anchor just be dropped entirely?

thx. May do, if I have to re-push

Makes sense. Still think "IMPORT_VS_ENV" is clearer than "SET_UP_VS" but it's tangential to this PR.

thx, done by using your patch

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.

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)

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.

https://github.com/bitcoin/bitcoin/blob/b65ff0e5a1fd4ea2ae75e204729b8008c4ebb9ab/.github/workflows/ci.yml#L236

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.