The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32672.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34548 (ci: Add and use ci-windows-cross.py helper by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

cc @hebasto @hodlinator @davidgumberg for Concept ACK / NACK.

Concept ACK (but my PowerShell skill is 0).

@fanquake you are currently registered as a N-A-C-K.

Concept ACK.

Defining a new custom job.defaults.run.shell for all Windows jobs allows us to revert f861919 and use the same shell across all steps. This is especially convenient for reproducing failures locally.

1. The output in the "Get tool information" step has changed:

I just added this as a nice to have but happy to remove.

2. The exit code is not propagated to the action's status.

Unfortunately I think this is just how it works. When there is a failure it will always use code 1. Without having to write code that checks error codes and returns those codes each time we call a native executable I don't think it's possible to have the same error code.

This is especially convenient for reproducing failures locally.

Just a general note on CI reproducibility: My recommendation would be to put as little code into vendor-specific locked-in yaml files and instead place CI code into vendor-agnostic scripts, so that they can be re-used outside of that vendor.

I understand that it is nice to have the CI log split into sections, but this should also be possible with a script that allows several entry points: ./ci.py install; ./ci.py build; ./ci.py test; (or similar)

Just an unrelated general note. Anything is fine here and it is probably best to leave the decisions to the Windows devs here.

After reading @maflcko's comment (which I agree with) and speaking with @hebasto offline I think it would be best that we change this to use powershell for all run steps on Windows jobs and look at condensing steps down into one or more scripts to aid in local reproducibility. As it stands, for someone to reproduce these steps locally they would have to jump between two shells. As I understand it, the change to use bash on windows was for the nicer fail fast behaviour but if we can get that in powershell now we should probably use the more native windows shell.

Marking as draft for now until I rework.

As I understand it, the change to use bash on windows was for the nicer fail fast behaviour but if we can get that in powershell now we should probably use the more native windows shell.

bash is also easier for non-windows devs to read and modify, so an alternative would also be python or rust as the ci runner script, but no strong opinion, I'd say anything is fine here.

@hebasto is there any reason we need Powershell at all? Can we just use bash (or another language) everywhere?

@hebasto is there any reason we need Powershell at all?

In my opinion, it is convenient to be able to reproduce CI commands locally without needing to install additional tools.

Can we just use bash (or another language) everywhere?

There are no technical obstacles to doing so.

I think it would be best that we change this to use powershell for all run steps on Windows jobs and look at condensing steps down into one or more scripts to aid in local reproducibility. As it stands, for someone to reproduce these steps locally they would have to jump between two shells. As I understand it, the change to use bash on windows was for the nicer fail fast behaviour but if we can get that in powershell now we should probably use the more native windows shell.

Concept ACK, seems reasonable to write the script in powershell and invoke it in the vendor ci.yml. I think the script should be moved out of the vendor-specific ci.yml as suggested by @maflcko.

Given that the script is mostly sequential command invocation, I'm not sure if there's much benefit from rewriting it in python or another language, and agree with @hebasto that for reproducibility it would be nice to have it in powershell. If there is a good reason to not have the CI script in powershell, maybe it would suffice to have a powershell script that does something like winget install Python.Python.3 and runs a python script.

Well, python is required and already installed on the GHA hosts, so the local runner will have to have it (pre-)installed in some way or another. I think it is fine to ignore the "install" part and only extract the "run" part, but anything is fine here. I'll likely not going to review this pull, so I don't want to give too much feedback 😅

I've implemented the concept of my idea in #34500

Did the rest in #34548

This leaves only:

      - &SET_UP_VS
        name: Set up VS Developer Prompt
        shell: pwsh -Command "$PSVersionTable; $PSNativeCommandUseErrorActionPreference = $true; $ErrorActionPreference = 'Stop'; & '{0}'"
        run: |
          $vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
          $installationPath = & $vswherePath -latest -property installationPath
          & "${env:COMSPEC}" /s /c "`"$installationPath\Common7\Tools\vsdevcmd.bat`" -arch=x64 -no_logo && set" | foreach-object {
            $name, $value = $_ -split '=', 2
            echo "$name=$value" >> $env:GITHUB_ENV
          }

      - name: Get tool information
        shell: pwsh
        run: |
          cmake -version | Tee-Object -FilePath "cmake_version"
          Write-Output "---" 
          msbuild -version | Tee-Object -FilePath "msbuild_version"
          $env:VCToolsVersion | Tee-Object -FilePath "toolset_version"
          py -3 --version
          Write-Host "PowerShell version $($PSVersionTable.PSVersion.ToString())"
          bash --version

Not sure what to do about those. I think they can be left as-is? Or should something be done?

Isn't Set up VS Developer Prompt already done?

I think it's probably ok just just leave as is, rather than making this tiny PR. If someone is in that area it might be nice to have for consistency.

Isn't Set up VS Developer Prompt already done?

I don't know. I presume the CI will fail if the step is removed?

I think it's probably ok just just leave as is, rather than making this tiny PR.

Yeah, I think it is best to close. I tried to move it to Python, but given that this calls cmd.exe, one enters the cmd passing/parsing hell on Windows.

Commit fa71b94 didn't pass CI.

It would have been nice, because it would allow to remove all pwsh in commit fae91a8, but I guess we are stuck with it for now.

Actually, #34583 fully removes pwsh and also passes CI