Skip to content

[vcpkg] (Implementation) Possibility to control the compiler hash for binary caching#16632

Closed
klalumiere wants to merge 0 commit intomicrosoft:masterfrom
klalumiere:master
Closed

[vcpkg] (Implementation) Possibility to control the compiler hash for binary caching#16632
klalumiere wants to merge 0 commit intomicrosoft:masterfrom
klalumiere:master

Conversation

@klalumiere
Copy link
Contributor

Describe the pull request

Use optional environment variables VCPKG_CXX_COMPILER_HASH_CMAKE_EXPRESSION and VCPKG_C_COMPILER_HASH_CMAKE_EXPRESSION to control the compiler hash. If the variables are not defined, the default behavior remains.

Example of value for VCPKG_CXX_COMPILER_HASH_CMAKE_EXPRESSION:

export VCPKG_CXX_COMPILER_HASH_CMAKE_EXPRESSION='${CMAKE_CXX_COMPILER_ID}'
  • Which triplets are supported/not supported? Have you updated the CI baseline? The same. No.

  • Does your PR follow the maintainer guide? Yes

@PhoebeHui PhoebeHui added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Mar 10, 2021
@PhoebeHui PhoebeHui requested a review from ras0219-msft March 10, 2021 01:54
@PhoebeHui
Copy link
Contributor

@klalumiere, thanks for the PR!

@ras0219 @ras0219-msft, could you help review?

@ras0219-msft
Copy link
Contributor

Thanks for the PR!

First, the current implementation will only work on non-Windows platforms -- on Windows, we clean out the environment variables in order to produce more reliable builds between machines, so these variables will need to be explicitly leaked in via either adding to our "clean environment" list or the user also explicitly using VCPKG_KEEP_ENV_VARS.

Second, the entire need for running detect_compiler is to determine the compiler hashes, so if those are being specified in the environment we should avoid running it entirely. Instead, the user should provide all the information that we would have detected from that step (including compiler id and version). Additionally, this information must be part of the triplet instead of the environment in order to properly support multiple triplets (like host dependencies) in a single build.

Therefore, to directly fix these issues, I would suggest:

  1. Make these triplet properties
  2. Require all 6 values to be set if any are set ({compiler hash, compiler id, compiler version} x {C, CXX})
  3. Replace the detect_compiler invocation implementation to skip that step entirely if the information was provided as part of the triplet.

Alternatively, we could expose (effectively) --feature-flags=-compilertracking as a triplet setting, which would omit the detect compiler step entirely and rely on the hash of the triplet file + toolchain to distinguish package ABI. In this case, the user could add a comment to their triplet file containing any information they want to distinguish (such as compiler version). I think this is a more promising approach because it has the least interface churn.

@klalumiere
Copy link
Contributor Author

First, the current implementation will only work on non-Windows platforms -- on Windows,

Yeah I had thought about this: the plan was indeed to use VCPKG_KEEP_ENV_VARS.

Alternatively, we could expose (effectively) --feature-flags=-compilertracking as a triplet setting, which would omit the detect compiler step entirely and rely on the hash of the triplet file + toolchain to distinguish package ABI.

This is a great idea! I'll try to implement it soon. Thanks! 🙂

@ras0219-msft ras0219-msft marked this pull request as draft March 26, 2021 21:47
@ras0219-msft
Copy link
Contributor

Moving this to draft for now; please feel free to mark it ready for review or close when you feel it's appropriate :)

@klalumiere
Copy link
Contributor Author

@ras0219-msft
Actually, I've just realized, if I want to implement your suggestion, I'll need to make a pull request in vcpkg-tool, not here, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[vcpkg] Possibility to control the compiler hash for binary caching

3 participants