ci: Avoid toolset ambiguity that MSVC can’t handle #28905

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:231117-ci-win changing 1 files +49 −0

hebasto commented at 4:12 pm on November 17, 2023: member
This PR introduces a workaround, which is similar to the one removed in #28796, required to work with the new windows-2022 image version 20231115 properly.

Tested on the following image versions:
- 20231029.1.0
- 20231115.2.0
Fixes #28901.

ci: Avoid toolset ambiguity that MSVC can't handle

This change is required to work with the new windows-2022 image version
20231115 properly.

91d5bd8ac9

DrahtBot commented at 4:12 pm on November 17, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, TheCharlatan, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Nov 17, 2023
maflcko commented at 4:22 pm on November 17, 2023: member

Looks like this reverts 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da?
hebasto commented at 4:24 pm on November 17, 2023: member

Looks like this reverts 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da?

Different versions though.
hebasto commented at 5:39 pm on November 17, 2023: member
This PR CI runs:
- Version: 20231029.1.0: https://github.com/bitcoin/bitcoin/actions/runs/6906384370/job/18791294660
- Version: 20231115.2.0: https://github.com/bitcoin/bitcoin/actions/runs/6906384370/job/18792841149
maflcko commented at 3:38 pm on November 20, 2023: member

lgtm ACK 91d5bd8ac9a28725c735f8e6900bc85673bb190a , assuming it fixes the CI failures
TheCharlatan approved
TheCharlatan commented at 5:32 pm on November 20, 2023: contributor

utACK 91d5bd8ac9a28725c735f8e6900bc85673bb190a
pablomartin4btc approved
pablomartin4btc commented at 8:15 pm on November 20, 2023: member

utACK 91d5bd8ac9a28725c735f8e6900bc85673bb190a
hebasto merged this on Nov 21, 2023
hebasto closed this on Nov 21, 2023
hebasto deleted the branch on Nov 22, 2023
fanquake commented at 11:13 am on November 22, 2023: member

Do we know why this broke again? I guess this is something we’ll just have to maintain forever? That’s a shame, because it also adds ~30% runtime to this CI job.
hebasto commented at 11:28 am on November 22, 2023: member

Do we know why this broke again?

The underlying issue is labeled “Under Investigation”.

I guess this is something we’ll just have to maintain forever?

Or until Microsoft fixes the issue.

That’s a shame, because it also adds ~30% runtime to this CI job.

I agree with you.
fanquake commented at 11:56 am on November 22, 2023: member

Added to #28872 for backporting.
fanquake referenced this in commit 5eaa179f27 on Nov 22, 2023
fanquake referenced this in commit e4fef4ae65 on Nov 22, 2023
hebasto commented at 10:39 am on November 24, 2023: member
Do we know why this broke again? I guess this is something we’ll just have to maintain forever?

I’ve done a bit more research.

In GHA Windows images, it is an accepted practice to have different versions of MSVC toolsets being installed side-by-side.

For example, the current image 20231115 has the following toolsets installed:
- 14.29.16.11
- 14.37.17.7
- 14.38.17.8
The ilammy/msvc-dev-cmd action chooses the latest compiler toolset version by default.

However, when building vcpkg dependencies in the manifest mode, MSVC uses another “default” toolset version, which causes link errors like “fatal error C1900”.

After switching building vcpkg dependencies from the manifest mode to the manual one, there is no link errors anymore. (the manual mode does not support package versioning, therefore, a few adjustments are needed) @sipsorcery

Do you have any insights regarding the correct usage of the manifest mode on systems with side-by-side version MSVC toolsets?
hebasto commented at 6:10 pm on November 24, 2023: member

Do we know why this broke again? I guess this is something we’ll just have to maintain forever? That’s a shame, because it also adds ~30% runtime to this CI job.

Please see #28934.
sipsorcery commented at 6:07 pm on November 25, 2023: member

@sipsorcery

Do you have any insights regarding the correct usage of the manifest mode on systems with side-by-side version MSVC toolsets?

I don’t.

Whenever I encountered something similar I’d purge the vcpkg dependencies and rebuild so it would use the latest version of the msvc tools.

ChatGPT, on the other hand, has this to say on the matter:

In summary, using vcpkg with manifest mode in an environment with multiple MSVC toolsets requires careful configuration of your vcpkg manifest file, CMakeLists, and potentially custom triplets to ensure that the correct toolset and dependencies are used for your project.

With a bit more probing it does seem apparent that there’s no way to specify the compiler version for a vcpkg dependency. The custom triplets is a red herring and would require tweaking the config of every vcpkg installed library being used.

Even if there was a way to sepcify a compiler version with vcpkg it seems like this problem would still exist due to the MS bug linked above which chooses the wrong toolset for the compiler version.

I suspect this PR is as good as it gets until the bug is fixed.

utACK 91d5bd8ac9a28725c735f8e6900bc85673bb190a
hebasto commented at 6:49 pm on November 25, 2023: member

@sipsorcery

With a bit more probing it does seem apparent that there’s no way to specify the compiler version for a vcpkg dependency. The custom triplets is a red herring and would require tweaking the config of every vcpkg installed library being used.

Did you see #28934?

Type	Reviewers
ACK	maflcko, TheCharlatan, pablomartin4btc

Contributors
hebasto DrahtBot maflcko TheCharlatan pablomartin4btc fanquake sipsorcery

Labels
Tests

ci: Avoid toolset ambiguity that MSVC can’t handle #28905

Code Coverage

Reviews