Skip to content
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.

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

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

Merged
merged 8 commits into from
Dec 10, 2021

Conversation

dg0yt
Copy link
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

@cenit
Copy link
Contributor

cenit commented Dec 4, 2021

as a testament to how much uwp is used… unfortunately

@dg0yt
Copy link
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
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
Contributor

Related: #19818.

@dg0yt
Copy link
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
Copy link
Contributor

@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
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
Contributor Author

dg0yt commented Dec 6, 2021

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

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 8, 2021

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 8, 2021

@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
5 tasks
@PhoebeHui
Copy link
Contributor

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

@JackBoosY
Copy link
Contributor

Please merge to master first.

@BillyONeal
Copy link
Member

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

@dg0yt
Copy link
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
Member

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.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Dec 10, 2021
@JackBoosY
Copy link
Contributor

LGTM.

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

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
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants