-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[vcpk-ci] Trigger some test ports from vcpkg.cmake changes #23430
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:
scripts/test_ports/cmake-user/vcpkg.json
Valid values for the license field can be found in the documentation
Bumping the port version does trigger the rebuild. But this is not how |
why vcpkg.cmake is not part of abi hashing? i'd consider it as necessary, isn't it? |
Right. We only want to build what is affected by the curent change.
This is not a new observation. I guess there is an intention to avoid a world rebuild. (However, given that even documentation changes to maintainer scripts invalidate many artifacts, this strategy isn't applied consistently.) Anyways, changing this is out of scope for this PR. |
# The vcpkg.cmake toolchain file is not part of ABI hashing, | ||
# but changes must trigger at least some testing. | ||
Copy-Item "scripts/buildsystems/vcpkg.cmake" -Destination "scripts/test_ports/cmake-user" | ||
Copy-Item "scripts/buildsystems/vcpkg.cmake" -Destination "scripts/test_ports/vcpkg-ci-paraview" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vcpkg-ci-paraview builds nothing so rebuilding it won't do any testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the theory, and I'm open to remove this part.
I chose that port because paraview
has many dependencies which are used in specific non-standard feature configurations for different triplets via vcpkg-ci-paraview
.
So in practice, there is a reasonable chance to hit a feature combination for some dependency which is normally not built this way. The last CI run for my PR built paraview
for all triplets where not blocked by a cascade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok that is the idea. But I would simply copy it into zlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My approach suffers from the fact that it does not affect the ABI hash of the dependencies, so it will be ineffective when CI runs are triggered while everything is still cached - e.g. when updating a PR.
zlib cannot be touched (easily) in the same way because it would need an update to versions
.
We can only touch stuff which does not impact versions
:
- test ports (drop file) - allows scoping to a particular test port such as
cmake-user
- triplets (append checksum) - allows scoping to a particular triplet or to target triplets.
- cmake scripts (append checksum) - can trigger a world rebuild in CI only (
ports.cmake
)
it is, imho. Instead of copying vcpkg.cmake file here and there just to trigger an abi change, why not including it directly in the files computed for abi? |
It is out-of-scope. Make a separate PR if you want to change ABI hashing. |
/azp run |
I like the purpose of this PR and the simplicity of these changes, maybe the rest of the team has input on how to make this testing more comprehensive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change and think we should merge it: it's an improvement on the status quo. However, as @dg0yt points out it leaves behind somewhat inconsistent behavior because minor changes to scripts/cmake inconsistently trigger world rebuilds.
I think that ultimately, the things in scripts/cmake are 'logically' part of the tool. Remaining in this repo rather than getting moved to the tool repo is an accident of backwards compatibility support. However, we really should be treating changes to those like we would treat other tool changes. To that end, I think we need to:
- Establish some better means of testing tool changes without needing to go through the whole tool release process. This will likely result in some duplication of instructions of building the tool in this repo but outside of signing and packaging those have been pretty stable.
- Copy the existing contents of the scripts tree to the 'vcpkg-tool' repo, since that is their 'logical owner'.
- Amend the tool release process (for which we already trigger full builds anyway) to include publishing of any updates to scripts/cmake necessary.
Regardless of whether the team likes the proposal I outline above, this PR's change is an improvement on the status quo and should land even if we adopt such a proposal.
* master: (103 commits) [lmdb] don't use msvc parameters with non-msvc compiler (microsoft#23653) Fix <version> of Python in vcpkgTools.xml (microsoft#23751) [sciter] escape quotes (microsoft#23752) [fuzzylite] Fix Linux build (microsoft#23658) [cpp-redis] Fix ‘sleep_for’ is not a member of ‘std::this_thread’ (microsoft#23762) [nlohmann-json] Fix usage text (microsoft#23749) [fontconfig] Do not create symlinks (microsoft#23735) (microsoft#23736) [winsparkle] Fix header file and debug path (microsoft#23739) [arb] Support dynamic build (microsoft#23743) [aws-sdk-cpp] update to 1.9.220 (microsoft#23729) Fix the VS2022 'unstable' queues. (microsoft#23742) [xmlsec] Bump to 1.2.33 (microsoft#23733) [unicorn] update to latest version v1.0.3 (microsoft#23745) [libpq] Update version to 14.1 2 (microsoft#22516) [libnoise] Export CMake files (microsoft#23682) [vcpk-ci] Trigger some test ports from vcpkg.cmake changes (microsoft#23430) [nlohmann-json] Add option to control implicit conversions behaviour (microsoft#22409) [libjuice] Update to 0.9.8 (microsoft#23153) [libgeotiff] Update to 1.7.1 (microsoft#23446) [minizip-ng] Updated minizip version and fixed windows build for previous version (microsoft#23684) ...
What does your PR fix?
The vcpkg.cmake toolchain file is not part of ABI hashing, but changes must trigger at least some testing.
In particular, there must be a test with the minimum supported version of cmake, to avoid issues like VCPKG fails to configure when running through cmake configuration on linux. #23402.
Which triplets are supported/not supported? Have you updated the CI baseline?
all, 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?not needed