-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-43951: [CI][Python] Use GitHub Packages for vcpkg cache #44644
Conversation
|
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
|
c931193
to
f401684
Compare
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
1 similar comment
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
78ee981
to
8b4f8ab
Compare
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
2 similar comments
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
1 similar comment
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
63d255c
to
6730902
Compare
@github-actions crossbow submit wheel-manylinux-2-28-cp39-cp39-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit -g wheel java-jars |
Revision: 52b6ee9 Submitted crossbow builds: ursacomputing/crossbow @ actions-f4d0e5be13 |
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.
LGTM, just a minor nit and a question
@@ -53,4 +53,33 @@ if [ -f "${vcpkg_ports_patch}" ]; then | |||
echo "Patch successfully applied to the VCPKG port files!" | |||
fi | |||
|
|||
if [ -n "${GITHUB_TOKEN:-}" ] && \ | |||
[ -n "${GITHUB_REPOSITORY_OWNER:-}" ] && \ | |||
[ "${VCPKG_BINARY_SOURCES:-}" = "clear;nuget,GitHub,readwrite" ] ; then |
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.
in which case would VCPKG_BINARY_SOURCES
be different to clear;nuget,GitHub,readwrite
? on manylinux2014_aarch64
this wouldn't be present but not different, right?
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.
If VCPKG_BINARY_SOURCES
is not clear;nuget,GitHub,readwrite
(including not present case on manylinux2014_aarch64
), we don't configure NuGet automatically.
Ignoring not clear;nuget,GitHub,readwrite
VCPKG_BINARY_SOURCES
is for using other cache. For example, we may want to use local file cache for local archery docker run
. But it's out-of-scope of this PR and it's not tested. If we need it, we can work on it as a separated task.
Co-authored-by: Raúl Cumplido <[email protected]>
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.
Thanks for adding the proper secret handling, looks great!
I'll merge this. |
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit df40f7a. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
We're using only Docker level cache for vcpkg used for wheels. If we have any vcpkg related changes, all vcpkg ports are rebuilt. It's time consuming.
What changes are included in this PR?
Enable NuGet + GitHub Packages based cache. It's port level cache. So we don't need to rebuild all ports when we have any vcpkg related changes.
See also: https://learn.microsoft.com/en-us/vcpkg/consume/binary-caching-github-packages
NuGet + GitHub Packages based cache isn't enabled with manylinux2014 + aarch64. Because EPEL for CentOS 7 + aarch64 provides old Mono. (FYI: EPEL for CentOS 7 + x86_64 provides newer Mono.) We can't use old Mono to run NuGet on Linux.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.