Skip to content

ci: Set MSVC toolset version explicitly #28934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 28, 2023
Merged

ci: Set MSVC toolset version explicitly #28934

merged 3 commits into from
Nov 28, 2023

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Nov 24, 2023 •
edited
Loading

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:

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
ci: Run vcpkg with path prefix
4335e55 
The GHA VS installation includes its own vcpkg package manager, which is
available since VS 17.6. This change avoids any ambiguity about which
copy of vcpkg we run.
@hebasto
ci: Set MSVC toolset version explicitly
1a889f7 
This change avoids toolset incompatibilities that cause linker errors.
@hebasto
Revert "ci: Avoid toolset ambiguity that MSVC can't handle"
70100f8 
This reverts commit 91d5bd8.
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 24, 2023 •
edited
Loading

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 pablomartin4btc
Stale ACK sipsorcery

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Nov 24, 2023
pablomartin4btc
pablomartin4btc approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 70100f8 since I've reviewed to be reverted #28905.

@hebasto hebasto mentioned this pull request Nov 24, 2023
Merged
@sipsorcery
Copy link
Contributor

Does the CI job still cache the vcpkg dependencies? Will this PR still work if there a minor version update on the build image?

@hebasto
Copy link
Member Author

hebasto commented Nov 25, 2023

Does the CI job still cache the vcpkg dependencies?

It does.

Will this PR still work if there a minor version update on the build image?

I've added the toolset version to the cache key:

bitcoin/.github/workflows/ci.yml

Line 250 in 70100f8

key: ${{ github.job }}-vcpkg-binary-${{ hashFiles('vcpkg_commit', 'msbuild_version', 'toolset_version', 'build_msvc/vcpkg.json') }}

so the cache will be invalidated if it is changed.

@sipsorcery
Copy link
Contributor

sipsorcery commented Nov 25, 2023 •
edited
Loading

utACK 4335e55.

utACK 70100f8.

@DrahtBot DrahtBot requested a review from sipsorcery November 25, 2023 19:54
@hebasto
Copy link
Member Author

hebasto commented Nov 25, 2023

@sipsorcery

utACK 4335e55.

Wrong hash?

:)

@hebasto
Copy link
Member Author

hebasto commented Nov 27, 2023

cc @maflcko @fanquake

@maflcko
Copy link
Member

maflcko commented Nov 27, 2023

Seems fine to do this, if this makes CI faster and there are no major downsides

@fanquake fanquake merged commit c1b7332 into bitcoin:master Nov 28, 2023
@hebasto hebasto deleted the 231124-toolset branch November 28, 2023 10:55
@bitcoin bitcoin locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sipsorcery sipsorcery Awaiting requested review from sipsorcery

1 more reviewer

@pablomartin4btc pablomartin4btc pablomartin4btc approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hebasto @DrahtBot @sipsorcery @maflcko @pablomartin4btc @fanquake