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

[vcpkg] Add experimental $X_VCPKG_ASSET_SOURCES for source caching #13639

Merged
merged 11 commits into from
Jun 4, 2021

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Sep 21, 2020

Work in progress; subject to change. Based on the strategy in #12532.

This PR merges several other PRs; prefer reviewing these individual pieces instead:

This PR requires microsoft/vcpkg-tool#10

toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/system.process.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/binarycaching.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/binarycaching.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.fetch.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.fetch.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/downloads.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/downloads.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/binarycaching.cpp Outdated Show resolved Hide resolved
@BillyONeal
Copy link
Member

#12532 tries to achieve similar things, might want to coordinate with Jack

@JackBoosY JackBoosY self-assigned this Sep 22, 2020
@JackBoosY JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal This PR or Issue was filed by the vcpkg team. labels Sep 22, 2020
@JackBoosY
Copy link
Contributor

We may need to provide a way for mirror maintainers so that they can easily download all necessary source packages to their computers.

ras0219 pushed a commit to ras0219/vcpkg that referenced this pull request Sep 26, 2020
@ras0219 ras0219 force-pushed the dev/roschuma/azblob-src branch from 1a6498c to 48623b1 Compare September 26, 2020 08:51
@ras0219
Copy link
Contributor Author

ras0219 commented Sep 26, 2020

The current implementation is intended to upload all downloaded binaries to the mirror (using HTTP PUT) -- this obviously needs to be configurable in a productized version.

@ras0219 ras0219 changed the title [vcpkg] Add experimental $VCPKG_X_SOURCE_MIRROR_URL_TEMPLATE for source caching [vcpkg] Add experimental $VCPKG_X_READWRITE_MIRROR_URL_TEMPLATE for source caching Sep 26, 2020
@ras0219 ras0219 force-pushed the dev/roschuma/azblob-src branch 3 times, most recently from ddf6599 to 465cb16 Compare September 26, 2020 13:38
@ras0219 ras0219 force-pushed the dev/roschuma/azblob-src branch from 465cb16 to f3bd0cb Compare October 7, 2020 19:27
@JackBoosY
Copy link
Contributor

Fixes #11040.

strega-nil pushed a commit that referenced this pull request Nov 18, 2020
* [vcpkg] Add experimental x-azblob binary provider

* [vcpkg] Test azblob storage provider in CI

* [vcpkg] Address some CR comments from #13639

* [vcpkg] Fixup azure-pipelines

* [vcpkg] Fix regression where the downloaded package is purged before decompressing

* [vcpkg] Further refactor vcpkg::Downloads

* [vcpkg] Enable OSX for x-azblob testing

* [vcpkg] Reduce diff against master

* [vcpkg] Extract Downloads::details::split_uri_view

* [vcpkg] Address PR comments

* [vcpkg] Add testing and metrics for x-azblob

* [vcpkg] Add docs for x-azblob

This includes a note that it is currently experimental

* [vcpkg] Address CR comments

* [vcpkg] Revert pipeline changes except OSX to minimize disruption

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
@ras0219 ras0219 force-pushed the dev/roschuma/azblob-src branch from 9dbd341 to 9b8824b Compare December 3, 2020 21:55
@JackBoosY
Copy link
Contributor

I very much hope that this PR can be deployed within this year.

@theidexisted
Copy link
Contributor

Any update for this draft? It's such a great feature! @BillyONeal

@BillyONeal
Copy link
Member

@theidexisted Robert (@ras0219) has been busy with other things.

@ras0219
Copy link
Contributor Author

ras0219 commented Jan 26, 2021

Cross posting from #13941 (comment):

There are additional challenges to be faced with making git.exe produce stable archives across all OSes. Marking this as draft for now until those problems are solved.

git.exe uses internal code to produce .tar files, and it changed in 2.26 for repos with paths longer than 100 characters
We can make sure we're using a precise git version on Windows, but we can't easily update the git version on other platforms.
The .gz format has an OS field that can differ between Windows and non-Windows (note: msys gzip does produce "unix" tagged archives).

This is the primary issue holding up this PR; we need to either:

  1. Figure out how to produce a bit-for-bit identical archive of git repos reliably across different machines
  2. Commit to having a separate source mirroring solution for git repos with a separate transformation scheme. Such a system would also not be able to automatically populate the mirror and would in general be much clunkier than (1).

@ras0219 ras0219 force-pushed the dev/roschuma/azblob-src branch 4 times, most recently from c7364b2 to 28c3c0a Compare January 30, 2021 11:15
strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
* [vcpkg] Add experimental x-azblob binary provider

* [vcpkg] Test azblob storage provider in CI

* [vcpkg] Address some CR comments from microsoft#13639

* [vcpkg] Fixup azure-pipelines

* [vcpkg] Fix regression where the downloaded package is purged before decompressing

* [vcpkg] Further refactor vcpkg::Downloads

* [vcpkg] Enable OSX for x-azblob testing

* [vcpkg] Reduce diff against master

* [vcpkg] Extract Downloads::details::split_uri_view

* [vcpkg] Address PR comments

* [vcpkg] Add testing and metrics for x-azblob

* [vcpkg] Add docs for x-azblob

This includes a note that it is currently experimental

* [vcpkg] Address CR comments

* [vcpkg] Revert pipeline changes except OSX to minimize disruption

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
@ras0219 ras0219 marked this pull request as ready for review May 26, 2021 16:40
@ras0219
Copy link
Contributor Author

ras0219 commented May 28, 2021

Three errors in the previous CI:

  1. vamp-sdk's domain https://code.soundsoftware.ac.uk is using a newer root certificate than the ones available on our VMs; this causes a failure to download. This can be solved by upgrading the ca-certificates package, but for now I will mark it as failing on the current infra.
  2. asiosdk had a temporary download failure that should be fixed on re-run
  3. chartdir does not offer stable download URLs and is inappropriate for the vcpkg main catalog. I'll mark it as failing on all platforms for now, but we should probably remove it and notify the original submitter that they can host it privately if they prefer.

scripts/cmake/vcpkg_download_distfile.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_download_distfile.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_download_distfile.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_download_distfile.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_download_distfile.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_download_distfile.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_download_distfile.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_download_distfile.cmake Outdated Show resolved Hide resolved
@ras0219 ras0219 marked this pull request as draft May 29, 2021 07:06
@ras0219 ras0219 marked this pull request as ready for review June 2, 2021 00:21
@JackBoosY
Copy link
Contributor

Looks all passed.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @ras0219

@ras0219-msft ras0219-msft merged commit 8c497bb into microsoft:master Jun 4, 2021
@devjgm
Copy link

devjgm commented Jun 5, 2021

FYI, my project is seeing some build errors after this merge that look possibly related. The error log is here: googleapis/google-cloud-cpp#6699

@cenit
Copy link
Contributor

cenit commented Jun 5, 2021

I have problems with downloads after this merge, closely related to the old (and somehow closed but not fixed #7207)

Building package opencv3[contrib,core,dnn,flann,jpeg,png,quirc,tiff,vtk,webp]:x64-windows...
-- Downloading https://github.com/opencv/opencv/archive/3.4.13.tar.gz -> opencv-opencv-3.4.13.tar.gz...
[DEBUG] Feature flag 'binarycaching' unset
[DEBUG] Feature flag 'manifests' unset
[DEBUG] Feature flag 'compilertracking' unset
[DEBUG] Feature flag 'registries' unset
[DEBUG] Feature flag 'versions' unset
[DEBUG] BuiltinRegistry initialized with: ""
[DEBUG] Using vcpkg-root: C:\vcpkg
[DEBUG] Using installed-root: C:\vcpkg\installed
[DEBUG] BuiltinRegistry initialized with: ""
[DEBUG] Using buildtrees-root: C:\vcpkg\buildtrees
[DEBUG] Using downloads-root: C:\vcpkg\downloads
[DEBUG] Using packages-root: C:\vcpkg\packages
[DEBUG] Using scripts-root: C:\vcpkg\scripts
[DEBUG] Using ports-root: C:\vcpkg\ports
[DEBUG] Using versions-root: C:\vcpkg\versions
[DEBUG] Downloading https://github.com/opencv/opencv/archive/3.4.13.tar.gz
Failed to download from mirror set:
https://github.com/opencv/opencv/archive/3.4.13.tar.gz: WinHttpQueryDataAvailable() failed: 12002

[DEBUG] C:\A\1\121\s\src\vcpkg\base\downloads.cpp(511)
[DEBUG] Exiting after 40014902 us (40014620 us)

CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:220 (message):
      
      Failed to download file.

I will revive #18171 hoping for merge

@autoantwort
Copy link
Contributor

FYI, my project is seeing some build errors after this merge that look possibly related. The error log is here: googleapis/google-cloud-cpp#6699

Was fixed by #18283 right now

endforeach()
endif()
vcpkg_execute_in_download_mode(
COMMAND "$ENV{VCPKG_COMMAND}" x-download "${downloaded_file_path}" "${vcpkg_download_distfile_SHA512}" ${urls} ${request_headers} --debug
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ras0219-msft This change introduced the use of the x-download vcpkg tool command which was not availabe in version 2021-01-13-d67989bce1043b98092ac45996a8230a059a2d7e of the tool. This already created a lot of issues, e.g. #18676. IMO this change needs to be complemented by use of

vcpkg_minimum_required(VERSION 2021-05-05)

either here where the new command is used, or globally in ports.cmake where the minimum is 2021-01-13 at the moment.

(I don't want to send a PR blindly because it will rebuild the world.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while you are very kind in principle, experience has already demonstrated many times that already merged PRs are rarely seen by vcpkg team.
i’d open a new issue at minimum, or send a PR anyway at best.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cenit Not really, I think we should force the user to update vcpkg and download vcpkg binary using bootstrap.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were probably more than 10 issues related to x-download, and none was taken as a reason to demand a change.

I would send a PR if it could be discussed before having another world rebuild blocking the CI pipeline. The script audits are heavy enough.

So as a start, I prefer to have another comment close to the offending change, adressing author, assignees and reviewers which normally remain subscribed. An issue or PR can follow.

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 info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants