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

[woff2] fix for static linking and alternative compiler toolchains #16392

Merged
merged 10 commits into from
Apr 7, 2021

Conversation

jwtowner
Copy link
Contributor

@jwtowner jwtowner commented Feb 24, 2021

This PR fixes static linking with the brotli port and fixes building woff2 with alternate compiler toolchains

Firstly, this change teaches the woff2 CMake configuration to understand vcpkg's custom unofficial-brotli pkg-config configuration in order to support static linking. This fixes a build failure on x64-osx, x64-linux triplet or other triplets when VCPKG_LIBRARY_LINKAGE is set to static.

Secondly, the CANONICAL_PREFIXES option for the woff2 CMake configuration has been changed to be on by default, otherwise
custom triplets or toolchains using alternate compilers such as Clang/LLVM or other versions of GCC will fail. Leaving CANONICAL_PREFIXES set to OFF causes -no-canonical-prefixes to be passed to the compiler, which prevents symlinked compiler toolchains from working correctly. If a user does actually need non-canonical prefixes, chances are they will have a custom triplet or toolchain file that passes in -no-canonical-prefixes as a CFLAG for every port, and so setting it to ON here is a better default for vcpkg.

EDIT: One other change I forgot to mention is that this specifies the BUNDLE DESTINATION parameter when calling install in CMakeLists.txt, setting it to the same path as the RUNTIME DESTINATION. The BUNDLE DESTINATION parameter is required for this to build on iOS and in some cases macOS, and does not affect other platforms.

Triplets fixed by this PR:

arm-ios
arm-linux
arm64-ios
arm64-linux
arm64-mingw-static
arm64-osx
arm64-windows-static
ppc64le-linux
s390x-linux
wasm32-emscripten
x64-ios
x64-openbsd
x64-osx
x64-linux
x64-mingw-static
x64-windows-static
x86-mingw-static
x86-windows-static
x86-windows-static-md

First, this change teaches the woff2 CMake configuration to
understand the vcpkg's custom unofficial-brotli pkg-config
configuration in order to support static linking. This fixes a
build failure on x64-linux or other triplets when
VCPKG_LIBRARY_LINKAGE is set to static.

Secondly, the CANONICAL_PREFIXES option for the woff2 CMake
configuration has been changed to be on by default, otherwise
custom triplets or toolchains using alternate compilers such as
Clang/LLVM or other versions of GCC will fail.
Leaving CANONICAL_PREFIXES set to OFF causes
-no-canonical-prefixes to be passed to the compiler, which
prevents symlinked compiler toolchains from working correctly.
If a user does actually need non-canonical prefixes, chances are
they will have a custom triplet or toolchain file that passes in
-no-canonical-prefixes as a CFLAG for every port, and so setting
it to ON here is a better default for vcpkg.
@jwtowner
Copy link
Contributor Author

jwtowner commented Feb 24, 2021

Sorry, I'm not sure what to do to get x86_windows Validate version files build step to pass in this case. Running the recommended command vcpkg x-add-version --all doesn't change any of the version files. Looking at versions/w-/woff2.json, I don't see anything off, but I must be missing something. Any ideas? Thanks!

EDIT: I muddled my way through. Updated the control file and reran vcpkg x-add-version woff2 --overwrite-version.

@cenit
Copy link
Contributor

cenit commented Feb 24, 2021

strange that you have fixed many triplets but no changes are required in ci.baseline.txt
is woff2 not tested in CI?

@jwtowner
Copy link
Contributor Author

jwtowner commented Feb 24, 2021

@cenit

strange that you have fixed many triplets but no changes are required in ci.baseline.txt
is woff2 not tested in CI?

Good question. Looking at ci.baseline.txt, I see:

woff2:x64-linux=fail
woff2:x64-osx=fail
woff2:x64-windows-static=fail
woff2:x64-windows-static-md=fail

Should I remove them from that file and submit it?

@JackBoosY JackBoosY added category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Feb 25, 2021
@cenit
Copy link
Contributor

cenit commented Feb 25, 2021

the system should have already told you...
please check in ci logs that the checks are performed. i will also check it later

@jwtowner
Copy link
Contributor Author

jwtowner commented Feb 25, 2021

@cenit @JonLiu1993

the system should have already told you...
please check in ci logs that the checks are performed. i will also check it later

Done! Should just be the stalled x64_osx CI build holding things up now.

@JackBoosY
Copy link
Contributor

Please remake the patch file.

@JackBoosY
Copy link
Contributor

@jwtowner Seems you revert my suggestion?

@jwtowner
Copy link
Contributor Author

@jwtowner Seems you revert my suggestion?

Sorry, botched patch regenertion somehow, trying again.

@cenit
Copy link
Contributor

cenit commented Feb 25, 2021

@BillyONeal is CI system not triggering "FIXED, REMOVE FROM FAILING" in the PR pipeline anymore?
Port was already fixed but not tested until manually removed from ci.baseline.txt

@BillyONeal
Copy link
Member

@BillyONeal is CI system not triggering "FIXED, REMOVE FROM FAILING" in the PR pipeline anymore?

Correct; we did this to remove the need to have tombstones be a concept when binary caching was productized; instead we build everything every night now and someone is supposed to look at those and fix them regularly.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 26, 2021
@cenit
Copy link
Contributor

cenit commented Feb 26, 2021

Correct; we did this to remove the need to have tombstones be a concept when binary caching was productized; instead we build everything every night now and someone is supposed to look at those and fix them regularly.

@BillyONeal ok looks reasonable. But why is not the Support manifest tag treated the same way? If a port is ever going to be fixed, CI will never notice it automatically, always skipping it and leaving the burden on the author (which I am sure will forget it)

@vicroms vicroms merged commit 9d80704 into microsoft:master Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants