don't use triplet file hash. Hash variables instead#596
don't use triplet file hash. Hash variables instead#596autoantwort wants to merge 1 commit intomicrosoft:mainfrom
Conversation
428cebc to
7dc8801
Compare
7dc8801 to
64f065f
Compare
| print_variables("15fe3605-2429-46b9-b6b1-c5c008621f79") | ||
| vcpkg_triplet_file(${VCPKG_TRIPLET_ID}) | ||
| print_variables("9dbc61d9-e274-40b4-b38a-4d2d3fe91341") |
There was a problem hiding this comment.
I think you also need to probe the environment before and after the triplet.
There was a problem hiding this comment.
Maybe, but for example you don't want to have the PATH in the abi. Another solution would be that you have to set a local variable to the value of the environment variable.
Do you have an example in which a environment variable should be included?
There was a problem hiding this comment.
You need to include whatever the triplet sets. So the only way to do that is comparing before/after and filtering unrelevant variables out.
There was a problem hiding this comment.
A problem would be that if a user sets a env variable outside of the triplet and uses this variable inside a port, this would be not caught (But this is probably impossible to detect. Except we include all env vars in the hash, but that would make the binarycache useless).
You need to include whatever the triplet sets. So the only way to do that is comparing before/after and filtering unrelevant variables out.
And if someone appends something to the PATH we should only hash the newly added stuff.
And if I look at
https://github.com/Neumann-A/my-vcpkg-triplets/blob/99d32a61fe890db22cc668fb4327891aa42f2699/armabi-v7a.cmake#L6
we should remove ${CMAKE_CURRENT_LIST_DIR} from the variable values before hashing.
There was a problem hiding this comment.
https://github.com/microsoft/vcpkg/pull/15053/files#diff-e1605373cd9d4822d5c5db0dbeef7008e0657895640b0639f4a3ca5e72d40407 has code to extract changes in the environment from a batch script. You basically need something similar
There was a problem hiding this comment.
Yeah I can use the code I already have. What I mean is that for example here:
https://github.com/Neumann-A/my-vcpkg-triplets/blob/99d32a61fe890db22cc668fb4327891aa42f2699/x64-windows-llvm.port.cmake#L22
you only want ${POSSIBLE_LLVM_BIN_DIR} gets hashed and not the whole ENV{PATH}
There was a problem hiding this comment.
And POSSIBLE_LLVM_BIN_DIR gets hashed anyways. Hm, do we have an example where it would be really necessary to hash changes of the env?
There was a problem hiding this comment.
Maybe we need something similar to VCPKG_ENV_PASSTHROUGH_UNTRACKED.
Something like VCPKG_TRIPLET_VARS_UNTRACKED and VCPKG_TRIPLET_ENV_VARS_(UN)TRACKED to explicitly enable/disable tracking of some (env) vars. Currently vcpkg just tracks everything from the triplet
really necessary to hash changes to the env?
Setting any variable which is related to building but not detected by the compiler detection: AR/ASM/FC
BillyONeal
left a comment
There was a problem hiding this comment.
Other than my ask for testing of a new parser, I think this from a code perspective is fine.
I would like to see @ras0219 / @ras0219-msft comment on what the product / ux implications may be before merging.
|
|
||
| if (opt_triplet_hashes) | ||
| { | ||
| auto parse_variables = [port_start, port_end](StringView start_guid, auto callback) { |
There was a problem hiding this comment.
New parser should be extracted and tested. (And maybe use ParserBase et al. but since this is parsing our own content I don't think that's strictly necessary)
| block_end = std::find(block_start, port_end, BLOCK_END_GUID); | ||
| } | ||
|
|
||
| if (opt_triplet_hashes) |
There was a problem hiding this comment.
| if (opt_triplet_hashes) | |
| if (auto triplet_hashes = opt_triplet_hashes.get()) |
| hasher->add_bytes(key.begin(), key.end()); | ||
| hasher->add_bytes(value.begin(), value.end()); | ||
| }); | ||
| opt_triplet_hashes.get()->push_back(hasher->get_hash()); |
There was a problem hiding this comment.
| opt_triplet_hashes.get()->push_back(hasher->get_hash()); | |
| triplet_hashes->push_back(hasher->get_hash()); |
|
I haven't dug 100% into the specifics, but I see one very large obstacle blocking this direction: paths in variables. Similar to my comment at microsoft/vcpkg#19984 (comment), I don't see how this approach can be implemented in a way that doesn't make the triplets incredibly machine- and project-location-specific. I'm also concerned about making the binary caching system even more opaque for users. Consider values like My counter-proposal to solve #19984 is to eventually introduce a more declarative (json) format for triplets with a narrower set of capabilities than CMake that can be precisely analyzed. It would be structurally clear which parts of the document apply to what ports and it would be possible to "see through" variable expansions to determine semantically significant substitions. Unfortunately, triplet file invalidation is not very high on our (the vcpkg core team) backlog of problems to solve, compared to bug fixing, docs improvements, artifacts, and PR review. |
No its not. It doesn't consider For me the problem seems to be Tracing all set/unset commands a list of triplet variables can be obtained without absolute paths and the correct hashing even if other files are included. There are no absolute paths since those don't get expanded from the variables and if there is an absolute path vcpkg can at least warn about it and its consequences (since then it is explicitly set as an absolute path). vcpkg could use I think this way is way better than the current approach since e.g. simple formatting changes in an if statement within the triplet won't affect everything anymore. |
|
Given that we don't see a path where Robert's comment ( #596 (comment) ) is resolved here we have closed this. If that could be resolved somehow we may be able to take a similar change. |
Fixes microsoft/vcpkg#19984