Skip to content

[vcpkg-cmake] Fix windows toolchain chainloading for uwp#21857

Merged
BillyONeal merged 8 commits intomicrosoft:masterfrom
dg0yt:cmake-uwp
Dec 10, 2021
Merged

[vcpkg-cmake] Fix windows toolchain chainloading for uwp#21857
BillyONeal merged 8 commits intomicrosoft:masterfrom
dg0yt:cmake-uwp

Conversation

@dg0yt
Copy link
Copy Markdown
Contributor

@dg0yt dg0yt commented Dec 4, 2021

  • What does your PR fix?

    Due to a wrong variable name, the windows toolchain file was not chainloaded for uwp. Note the empty VCPKG_CHAINLOAD_TOOLCHAIN_FILE passed here:
    [1/2] "D:/downloads/tools/cmake-3.21.1-windows/cmake-3.21.1-windows-i386/bin/cmake.exe" -S "D:/buildtrees/libjxl/src/v0.6.1-bf1b66f693.clean" -B "../../arm-uwp-dbg" "... "-DCMAKE_SYSTEM_NAME=WindowsStore" ... "-DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=" "-DVCPKG_TARGET_TRIPLET=arm-uwp" ...

    Without the chainloading, the VCPKG_C_FLAGS etc. are not added to CMAKE_C_FLAGS etc. This is even detected during cmake configuration:

    CMake Warning:
Manually-specified variables were not used by the project:
 ...
 VCPKG_CXX_FLAGS
 VCPKG_CXX_FLAGS_DEBUG
 VCPKG_CXX_FLAGS_RELEASE
 VCPKG_C_FLAGS
 VCPKG_C_FLAGS_DEBUG
 VCPKG_C_FLAGS_RELEASE

    This hurts twice on uwp because it often needs extra definitions or flags to turn of warnings/errors.
    Current case: [lodepng,lodepng-c, libjxl] Update, merge lodepng-c into lodepng #21846 build https://dev.azure.com/vcpkg/public/_build/results?buildId=64141&view=results

    I couldn't resist switching to the common definitions for targets in order to deduplicate target detection.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all (fixing uwp), no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    yes

@dg0yt dg0yt mentioned this pull request Dec 4, 2021
Closed
@cenit
Copy link
Copy Markdown
Contributor

cenit commented Dec 4, 2021

as a testament to how much uwp is used… unfortunately

@dg0yt dg0yt mentioned this pull request Dec 5, 2021
Closed
@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Dec 5, 2021

The x64_windows_static_md error is subject of #21840, or eventually #21846 (which led into this PR).

@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Dec 5, 2021

CC @BillyONeal @ras0219-msft for accelerated review, if uwp matters.

@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Dec 6, 2021
@JackBoosY
Copy link
Copy Markdown
Contributor

JackBoosY commented Dec 6, 2021

Related: #19818.

@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Dec 6, 2021

Related: #19818.

@JackBoosY I don't think #19818 is related to this PR. Maybe you meant to add the comment to #21507?

JackBoosY
JackBoosY suggested changes Dec 6, 2021
View reviewed changes
@JackBoosY
Copy link
Copy Markdown
Contributor

JackBoosY commented Dec 6, 2021

@dg0yt For uwp, there are some problems when using vcpkg-cmake, as my comment.
But for this PR, it is more like a "cleaning" job.

@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Dec 6, 2021

@dg0yt For uwp, there are some problems when using vcpkg-cmake, as my comment. But for this PR, it is more like a "cleaning" job.

Not just cleaning: f02227e fixes a serious port bug.

@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Dec 6, 2021

(I do separate commits to facilitate reviews. Please make use of it while reviewing.)

Neumann-A
Neumann-A reviewed Dec 6, 2021
View reviewed changes
@JackBoosY JackBoosY mentioned this pull request Dec 7, 2021
Closed
Neumann-A
Neumann-A reviewed Dec 7, 2021
View reviewed changes
@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Dec 8, 2021

@BillyONeal Another vcpkg ci exception again on agent vcpkg-eg-mac-02, https://dev.azure.com/vcpkg/public/_build/results?buildId=64286&view=logs&j=7b75bd19-17d3-53d4-00fd-23f1a49a8ba4&t=a23dba4e-3a62-59dc-a73c-a7222676d7a4.

@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Dec 8, 2021 •
edited
Loading

@BillyONeal Even worse, the windows builds are green despite the build failing for qtinterfaceframework. This even happened in yesterdays run. Failure logs are uploaded for all triplets.

@dg0yt dg0yt mentioned this pull request Dec 8, 2021
Open
5 tasks
@PhoebeHui
Copy link
Copy Markdown
Contributor

PhoebeHui commented Dec 9, 2021

qt* failures in baseline has been fixed by #21925.

@JackBoosY
Copy link
Copy Markdown
Contributor

JackBoosY commented Dec 9, 2021

Please merge to master first.

@dg0yt dg0yt mentioned this pull request Dec 9, 2021
Closed
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 10, 2021

Ideally we would use the right _CRT_SECURE_NO_WARNINGS defines rather than disabling 4996 (which disables all library-emitted diagnostics)

@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Dec 10, 2021

Ideally we would use the right _CRT_SECURE_NO_WARNINGS defines rather than disabling 4996 (which disables all library-emitted diagnostics)

Well, do you want me to trigger another world rebuild for this PR, or do we want to address that for netcdf-c and possibly other ports in another PR?
Note that the flag was present before.

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 10, 2021

Note that the flag was present before.

:sigh: I would really like to see it fixed given that lots of validation tools will reject binaries built with /wd4996, but given that it was there before and doesn't seem to be the true focus of your PR I suppose it's OK if that's not addressed in this particular PR.

@BillyONeal BillyONeal requested a review from JackBoosY December 10, 2021 04:59
@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Dec 10, 2021
@JackBoosY
Copy link
Copy Markdown
Contributor

JackBoosY commented Dec 10, 2021

LGTM.

@BillyONeal BillyONeal merged commit 207af1c into microsoft:master Dec 10, 2021
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 10, 2021

Thanks for your help and clear explanations, @dg0yt!

@dg0yt dg0yt deleted the cmake-uwp branch December 11, 2021 05:13
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

@github-actions github-actions[bot] github-actions[bot] approved these changes

@strega-nil-ms strega-nil-ms Awaiting requested review from strega-nil-ms

+2 more reviewers

@Neumann-A Neumann-A Neumann-A left review comments

@JackBoosY JackBoosY JackBoosY approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JackBoosY JackBoosY

Labels

category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dg0yt @cenit @JackBoosY @PhoebeHui @BillyONeal @Neumann-A