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

1. 
   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.

2. 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
3. 
   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.

4. DrahtBot added the label Tests on Nov 17, 2023
5. 
   maflcko commented at 4:22 pm on November 17, 2023: member

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

   Looks like this reverts 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da?

   Different versions though.

7. 
   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
8. 
   maflcko commented at 3:38 pm on November 20, 2023: member

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

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

    utACK 91d5bd8ac9a28725c735f8e6900bc85673bb190a
13. hebasto merged this on Nov 21, 2023
14. hebasto closed this on Nov 21, 2023

    ---

15. hebasto deleted the branch on Nov 22, 2023
16. 
    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.
17. 
    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.

18. 
    fanquake commented at 11:56 am on November 22, 2023: member

    Added to #28872 for backporting.
19. fanquake referenced this in commit 5eaa179f27 on Nov 22, 2023
20. fanquake referenced this in commit e4fef4ae65 on Nov 22, 2023
21. 
    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?

22. 
    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.

23. 
    sipsorcery commented at 6:07 pm on November 25, 2023: contributor

    @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

24. 
    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?

25. fanquake referenced this in commit c1b7332441 on Nov 28, 2023
26. bitcoin locked this on Nov 24, 2024

Contributors
hebasto DrahtBot maflcko TheCharlatan pablomartin4btc fanquake sipsorcery

Labels
Tests