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

hebasto · 2023-11-17T16:12:08Z

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:

Fixes #28901.

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

DrahtBot · 2023-11-17T16:12:12Z

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.

maflcko · 2023-11-17T16:22:47Z

Looks like this reverts 5bd1b8d?

hebasto · 2023-11-17T16:24:50Z

Looks like this reverts 5bd1b8d?

Different versions though.

hebasto · 2023-11-17T17:39:27Z

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 · 2023-11-20T15:38:13Z

lgtm ACK 91d5bd8 , assuming it fixes the CI failures

TheCharlatan

utACK 91d5bd8

pablomartin4btc

utACK 91d5bd8

fanquake · 2023-11-22T11:13:27Z

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 · 2023-11-22T11:28:21Z

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 · 2023-11-22T11:56:55Z

Added to #28872 for backporting.

This change is required to work with the new windows-2022 image version 20231115 properly. Github-Pull: bitcoin#28905 Rebased-From: 91d5bd8

2f86d30 doc: update release notes for v26.0rc3 (fanquake) 3b6c7f2 doc: update manual pages for v26.0rc3 (fanquake) 3db4d1c build: bump version to v26.0rc3 (fanquake) 6045f38 build: Fix regression in "ARMv8 CRC32 intrinsics" test (Hennadii Stepanov) 5eaa179 ci: Avoid toolset ambiguity that MSVC can't handle (Hennadii Stepanov) 55af112 p2p: do not make automatic outbound connections to addnode peers (Jon Atack) 5e0bcc1 ci: Switch from `apt` to `apt-get` (Hennadii Stepanov) 437a531 ci: Update apt cache (Hennadii Stepanov) 1488648 pool: change memusage_test to use int64_t, add allocation check (Martin Leitner-Ankerl) bcc183c pool: make sure PoolAllocator uses the correct alignment (Martin Leitner-Ankerl) 7dda499 doc: regenerate example bitcoin.conf (fanquake) 5845331 doc: rewrite explanation for -par= (fanquake) Pull request description: Currently backports: * #28858 * #28895 (partial) * #28913 * #28905 * #28919 * #28925 Also includes changes for rc3, and reintegrating the release-notes. ACKs for top commit: hebasto: re-ACK 2f86d30, only #28919 backported since my [recent](#28872 (review)) review. TheCharlatan: ACK 2f86d30 Tree-SHA512: 43c91b344d37f582081ac184ac59cf76c741317b2b69a24fcd4287eefa8333e20c545e150798f4057d6f4ac8e70ed9cba1c8dd9777b11c1cf8992cce09108727

hebasto · 2023-11-24T10:39:23Z

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 · 2023-11-24T18:10:41Z

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 · 2023-11-25T18:07:54Z

@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 91d5bd8

hebasto · 2023-11-25T18:49:21Z

@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?

70100f8 Revert "ci: Avoid toolset ambiguity that MSVC can't handle" (Hennadii Stepanov) 1a889f7 ci: Set MSVC toolset version explicitly (Hennadii Stepanov) 4335e55 ci: Run vcpkg with path prefix (Hennadii Stepanov) Pull request description: This PR is an alternative to #28905 and reverts it. To avoid toolset version incompatibilities, which result in errors like this: ``` LINK : fatal error C1900: Il mismatch between 'P1' version '20230904' and 'P2' version '20221215' [D:\a\bitcoin\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj] LINK : fatal error LNK1257: code generation failed [D:\a\bitcoin\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj] LINK : fatal error LNK1327: failure during running link.exe [D:\a\bitcoin\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj] ``` it is enough to set it explicitly in the vcpkg triplet file (see the second commit). The `VCToolsVersion` environment variable is set by the `ilammy/msvc-dev-cmd` action. Please note that the #28905 is not [optimal](#28905 (comment)): > 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. ACKs for top commit: sipsorcery: utACK 70100f8. pablomartin4btc: ACK 70100f8 since I've reviewed to be reverted #28905. Tree-SHA512: 121a8e40c728060526f380b7946211b5d4eca8821bfe62e6451642ffdf95fe9ab7101e0cffa7f4a777bc9cf94278bb50c1b40b71768e1ac39801bb4831afeb90

ci: Avoid toolset ambiguity that MSVC can't handle

91d5bd8

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

DrahtBot added the Tests label Nov 17, 2023

This was referenced Nov 17, 2023

ci: failure in Windows MSVC build #28901

Closed

wallet: Deniability API (Unilateral Transaction Meta-Privacy) #27792

Closed

hebasto mentioned this pull request Nov 20, 2023

coins: make sure PoolAllocator uses the correct alignment #28913

Merged

ismaelsadeeq mentioned this pull request Nov 20, 2023

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Merged

TheCharlatan approved these changes Nov 20, 2023

View reviewed changes

pablomartin4btc approved these changes Nov 20, 2023

View reviewed changes

hebasto merged commit daa56f7 into bitcoin:master Nov 21, 2023

willcl-ark mentioned this pull request Nov 21, 2023

init: Add option for rpccookie permissions (replace 26088) #28167

Merged

hebasto deleted the 231117-ci-win branch November 22, 2023 10:45

hebasto mentioned this pull request Nov 22, 2023

[26.x] Changes for rc3 #28872

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Nov 22, 2023

ci: Avoid toolset ambiguity that MSVC can't handle

5eaa179

This change is required to work with the new windows-2022 image version 20231115 properly. Github-Pull: bitcoin#28905 Rebased-From: 91d5bd8

hebasto mentioned this pull request Nov 24, 2023

ci: Set MSVC toolset version explicitly #28934

Merged

bitcoin locked and limited conversation to collaborators Nov 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

hebasto commented Nov 17, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Nov 17, 2023 •

edited

Loading

Uh oh!

maflcko commented Nov 17, 2023

Uh oh!

hebasto commented Nov 17, 2023

Uh oh!

hebasto commented Nov 17, 2023

Uh oh!

maflcko commented Nov 20, 2023

Uh oh!

TheCharlatan left a comment

Uh oh!

pablomartin4btc left a comment

Uh oh!

fanquake commented Nov 22, 2023

Uh oh!

hebasto commented Nov 22, 2023

Uh oh!

fanquake commented Nov 22, 2023

Uh oh!

hebasto commented Nov 24, 2023

Uh oh!

hebasto commented Nov 24, 2023

Uh oh!

sipsorcery commented Nov 25, 2023

Uh oh!

hebasto commented Nov 25, 2023

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

hebasto commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

maflcko commented Nov 17, 2023

Uh oh!

hebasto commented Nov 17, 2023

Uh oh!

hebasto commented Nov 17, 2023

Uh oh!

maflcko commented Nov 20, 2023

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 22, 2023

Uh oh!

hebasto commented Nov 22, 2023

Uh oh!

fanquake commented Nov 22, 2023

Uh oh!

hebasto commented Nov 24, 2023

Uh oh!

hebasto commented Nov 24, 2023

Uh oh!

sipsorcery commented Nov 25, 2023

Uh oh!

hebasto commented Nov 25, 2023

Uh oh!

Uh oh!

hebasto commented Nov 17, 2023 •

edited

Loading

DrahtBot commented Nov 17, 2023 •

edited

Loading