Skip to content

Move toolchain decision making entirely into the tool#350

Merged
BillyONeal merged 2 commits intomicrosoft:mainfrom
ras0219-msft:dev/roschuma/uwp-toolchain
Feb 17, 2022
Merged

Move toolchain decision making entirely into the tool#350
BillyONeal merged 2 commits intomicrosoft:mainfrom
ras0219-msft:dev/roschuma/uwp-toolchain

Conversation

@ras0219-msft
Copy link
Copy Markdown
Collaborator

@ras0219-msft ras0219-msft commented Feb 12, 2022

This PR is a needed tool change to enable microsoft/vcpkg#22831 and comes with the added benefit of removing duplicate toolchain selection logic across the many helper scripts. With this, ports and helpers may always assume that VCPKG_CHAINLOAD_TOOLCHAIN_FILE is defined to the correct toolchain to pull in.

+@Neumann-A for review

@ras0219-msft ras0219-msft mentioned this pull request Feb 12, 2022
Merged
@Neumann-A
Copy link
Copy Markdown
Contributor

Neumann-A commented Feb 14, 2022

as said in microsoft/vcpkg#22831. Do you really want to move it into the tool? I mean the triplet would be the correct location to simply set VCPKG_CHAINLOAD_TOOLCHAIN_FILE. This requires no selection variable at all and would remove any selection code. Maybe also move the toolchains into the triplet dir (maybe in a subfolder called toolchains). This would make it more visible and obvious for users how vcpkg uses toolchains (scripts/toolchains is kind of an implementation detail for me instead of being user facing which toolchains should be).
I also think that setting VCPKG_CHAINLOAD_TOOLCHAIN_FILE scales better since you don't need to add cases somewhere in the future.

I mean yeah it would be a breaking change for some custom triplets but it would be quite simple to inform users that they need to set VCPKG_CHAINLOAD_TOOLCHAIN_FILE in their custom triplets.

@ras0219-msft
Copy link
Copy Markdown
Collaborator Author

ras0219-msft commented Feb 14, 2022

I don't think we can make that breaking change any time soon -- though we could consider making it deprecated. In that light, I think this change is a pure win. Even with this change, we could add VCPKG_CHAINLOAD_TOOLCHAIN_FILE to all of the in-box triplets and everything will work out just fine. It also helps incrementally move us in that direction by allowing all other components in the system to act as though we were already there.

BillyONeal
BillyONeal approved these changes Feb 17, 2022
View reviewed changes
@ras0219-msft @BillyONeal
Update src/vcpkg/build.cpp
1c287f9 
Co-authored-by: Billy O'Neal <bion@microsoft.com>
@BillyONeal BillyONeal merged commit f3841c4 into microsoft:main Feb 17, 2022
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Feb 24, 2022

I'm pretty sure this change is what broke a lot of the UWP ports in

Should we revert this for now?

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Feb 24, 2022

Indeed:

PS C:\Dev\vcpkg> git -C ..\vcpkg-tool rev-parse HEAD
f562635b3b82e70f252844e94767d0907e2f48e9
PS C:\Dev\vcpkg> .\vcpkg.exe x-ci-clean
Starting vcpkg CI clean
Clearing contents of C:\Dev\vcpkg\buildtrees
Clearing contents of C:\Dev\vcpkg\installed
Clearing contents of C:\Dev\vcpkg\packages
Completed vcpkg CI clean
PS C:\Dev\vcpkg> ..\vcpkg-tool\out\build\x64-Debug\vcpkg.exe install expat:x64-uwp --no-binarycaching
Computing installation plan...
The following packages will be built and installed:
    expat[core]:x64-uwp -> 2.4.1
Detecting compiler hash for triplet x64-uwp...
Starting package 1/1: expat:x64-uwp
Building package expat[core]:x64-uwp...
-- Using cached libexpat-libexpat-a28238bdeebc087071777001245df1876a11f5ee.tar.gz.
-- Extracting source C:/Dev/vcpkg-downloads/libexpat-libexpat-a28238bdeebc087071777001245df1876a11f5ee.tar.gz
-- Applying patch pkgconfig.patch
-- Using source at C:/Dev/vcpkg/buildtrees/expat/src/876a11f5ee-539179dcdb.clean
-- Configuring x64-uwp
-- Building x64-uwp-dbg
-- Building x64-uwp-rel
-- Fixing pkgconfig file: C:/Dev/vcpkg/packages/expat_x64-uwp/lib/pkgconfig/expat.pc
-- Using cached msys-mingw-w64-i686-pkg-config-0.29.2-3-any.pkg.tar.zst.
-- Using cached msys-mingw-w64-i686-libwinpthread-git-9.0.0.6373.5be8fcd83-1-any.pkg.tar.zst.
-- Using msys root at C:/Dev/vcpkg-downloads/tools/msys2/9a1ec3f33446b195
-- Fixing pkgconfig file: C:/Dev/vcpkg/packages/expat_x64-uwp/debug/lib/pkgconfig/expat.pc
-- Installing: C:/Dev/vcpkg/packages/expat_x64-uwp/share/expat/copyright
-- Performing post-build validation
-- Performing post-build validation done
Installing package expat[core]:x64-uwp...
Elapsed time for package expat:x64-uwp: 28.44 s

Total elapsed time: 33.96 s

The package expat provides CMake targets:

    find_package(expat CONFIG REQUIRED)
    target_link_libraries(main PRIVATE expat::expat)

PS C:\Dev\vcpkg> git -C ..\vcpkg-tool rev-parse HEAD
f3841c4f3e04885a73c4ab9db7b396b1e32f9cb6
PS C:\Dev\vcpkg> .\vcpkg.exe x-ci-clean
Starting vcpkg CI clean
Clearing contents of C:\Dev\vcpkg\buildtrees
Clearing contents of C:\Dev\vcpkg\installed
Clearing contents of C:\Dev\vcpkg\packages
Completed vcpkg CI clean
PS C:\Dev\vcpkg> ..\vcpkg-tool\out\build\x64-Debug\vcpkg.exe install expat:x64-uwp --no-binarycaching
Computing installation plan...
The following packages will be built and installed:
    expat[core]:x64-uwp -> 2.4.1
Detecting compiler hash for triplet x64-uwp...
Starting package 1/1: expat:x64-uwp
Building package expat[core]:x64-uwp...
-- Using cached libexpat-libexpat-a28238bdeebc087071777001245df1876a11f5ee.tar.gz.
-- Extracting source C:/Dev/vcpkg-downloads/libexpat-libexpat-a28238bdeebc087071777001245df1876a11f5ee.tar.gz
-- Applying patch pkgconfig.patch
-- Using source at C:/Dev/vcpkg/buildtrees/expat/src/876a11f5ee-539179dcdb.clean
-- Configuring x64-uwp
-- Building x64-uwp-dbg
-- Building x64-uwp-rel
-- Fixing pkgconfig file: C:/Dev/vcpkg/packages/expat_x64-uwp/lib/pkgconfig/expat.pc
-- Using cached msys-mingw-w64-i686-pkg-config-0.29.2-3-any.pkg.tar.zst.
-- Using cached msys-mingw-w64-i686-libwinpthread-git-9.0.0.6373.5be8fcd83-1-any.pkg.tar.zst.
-- Using msys root at C:/Dev/vcpkg-downloads/tools/msys2/9a1ec3f33446b195
-- Fixing pkgconfig file: C:/Dev/vcpkg/packages/expat_x64-uwp/debug/lib/pkgconfig/expat.pc
-- Installing: C:/Dev/vcpkg/packages/expat_x64-uwp/share/expat/copyright
-- Performing post-build validation
The following DLLs do not have the App Container bit set:

    C:/Dev/vcpkg/packages/expat_x64-uwp/debug/bin/libexpatd.dll
    C:/Dev/vcpkg/packages/expat_x64-uwp/bin/libexpat.dll

This bit is required for Windows Store apps.
Found 1 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile:
    C:\Dev\vcpkg\ports\expat\portfile.cmake
-- Performing post-build validation done
Installing package expat[core]:x64-uwp...
Elapsed time for package expat:x64-uwp: 5.599 s

Total elapsed time: 11.23 s

The package expat provides CMake targets:

    find_package(expat CONFIG REQUIRED)
    target_link_libraries(main PRIVATE expat::expat)

PS C:\Dev\vcpkg>

BillyONeal added a commit that referenced this pull request Feb 24, 2022
@BillyONeal
Revert "Move toolchain decision making entirely into the tool (#350)"
536ca0a 
This reverts commit f3841c4.
@BillyONeal BillyONeal mentioned this pull request Feb 24, 2022
Closed
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Feb 24, 2022
@BillyONeal
Add Z_ to VCPKG_CHAINLOAD_TOOLCHAIN_FILE to workaround the regression…
ccc90b0 
… caused by microsoft#350
@BillyONeal BillyONeal mentioned this pull request Feb 24, 2022
Merged
BillyONeal added a commit that referenced this pull request Feb 24, 2022
@BillyONeal
Add Z_ to VCPKG_CHAINLOAD_TOOLCHAIN_FILE to workaround the regression…
46569d4 
… caused by #350 (#379)
@BillyONeal BillyONeal mentioned this pull request Nov 14, 2022
Merged
BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request May 22, 2025
@BillyONeal
Prohibit calls to z_vcpkg_select_default_vcpkg_chainload_toolchain
8fbad24 
In microsoft#45524 we the vcpkg team observe that z_vcpkg_select_default_vcpkg_chainload_toolchain should never be called after microsoft/vcpkg-tool#350 . We don't want to make a code review comment to leave it 'half broken', so we're submitting this change to make not editing that list consistent.
@BillyONeal BillyONeal mentioned this pull request May 22, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes

+1 more reviewer

@Neumann-A Neumann-A Neumann-A approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ras0219-msft @Neumann-A @BillyONeal