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 feature option 'withMirror' ⚡ #12532

Closed
wants to merge 48 commits into from

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Jul 22, 2020

This PR will add the use of mirror download address function, allow users to set the mirror address of ports and tools through parameters when building vcpkg to reduce download time.

usage:
To use vcpkg mirror:
set environment VCPKG_EXPERIMENTAL_MIRROR_URL to mirror address, example: 139.196.94.253.

To download all ports to mirror machine:

set VCPKG_EXPERIMENTAL_MIRROR_URL PATH_TO_DOWNLOADS
./vcpkg x-mirror

  • Modify build scripts
  • Add mirror related code
  • Add download related code
  • Test
  • Document

Thanks @ras0219-msft for the advice.

Closes #11040

@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 Jul 22, 2020
@JackBoosY JackBoosY changed the title [VCPKG] Add feature mirror [VCPKG] Add feature option 'useMirror' Jul 22, 2020
@JackBoosY JackBoosY changed the title [VCPKG] Add feature option 'useMirror' [VCPKG] Add feature option 'withMirror' Jul 23, 2020
@JackBoosY JackBoosY requested review from strega-nil and ras0219-msft and removed request for strega-nil July 23, 2020 09:17
@JackBoosY JackBoosY changed the title [VCPKG] Add feature option 'withMirror' [vcpkg] Add feature option 'withMirror':zap: Jul 23, 2020
@JackBoosY JackBoosY changed the title [vcpkg] Add feature option 'withMirror':zap: [vcpkg] Add feature option 'withMirror' ⚡ Jul 23, 2020
@JackBoosY JackBoosY changed the title [vcpkg] Add feature option 'withMirror' ⚡ [vcpkg] Add feature option 'withMirror' ⚡ Jul 23, 2020
@BillyONeal
Copy link
Member

Thanks, this is great 👍

I would like to see the control being done with an environment variable and/or command line option rather than compiling it into the tool with bootstrap, because once we start doing binary releases that won't work anymore.

@JackBoosY JackBoosY marked this pull request as ready for review July 24, 2020 05:38
@JackBoosY JackBoosY marked this pull request as draft July 24, 2020 05:38
@@ -12,134 +12,6 @@

namespace vcpkg::Downloads
{
#if defined(_WIN32)
static void winhttp_download_file(Files::Filesystem& fs,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since winhttp.h has conflict with wininet.h, I split this function into two files.

@JackBoosY
Copy link
Contributor Author

qt5-tools:

Qt5Guid.lib(qrhid3d11.obj) : error LNK2001: unresolved external symbol IID_IDXGIFactory2
Qt5Guid.lib(qrhid3d11.obj) : error LNK2001: unresolved external symbol IID_ID3D11DeviceContext1
Qt5Guid.lib(qrhid3d11.obj) : error LNK2001: unresolved external symbol IID_ID3DUserDefinedAnnotation

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Aug 25, 2020

clapack:

Building package clapack[core]:x64-osx... done
Installing package clapack[core]:x64-osx...
The following files are already installed in /Users/vagrant/Data/installed/x64-osx and are in conflict with clapack:x64-osx

Installed by lapack-reference:x64-osx
    lib/liblapack.a

@JackBoosY
Copy link
Contributor Author

qt5-tools:

Qt5Guid.lib(qrhid3d11.obj) : error LNK2001: unresolved external symbol IID_IDXGIFactory2
Qt5Guid.lib(qrhid3d11.obj) : error LNK2001: unresolved external symbol IID_ID3D11DeviceContext1
Qt5Guid.lib(qrhid3d11.obj) : error LNK2001: unresolved external symbol IID_ID3DUserDefinedAnnotation

@Neumann-A It seems that qt5-tools forgot to link to dxguid.lib?


if (ENV{VCPKG_EXPERIMENTAL_MIRROR_URL})
message(STATUS "Use vcpkg mirror $ENV{VCPKG_EXPERIMENTAL_MIRROR_URL}")
set(vcpkg_download_distfile_URLS "ftp://$ENV{VCPKG_EXPERIMENTAL_MIRROR_URL}/${vcpkg_download_distfile_SHA512}")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should do something reasonable in the vcpkg_download_distfile_SKIP_SHA512 case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without SHA512, vcpkg_download_distfile cannot splice the download URL.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I will merge this for you after #13038 lands; they both touch a lot of the same stuff but this one is a bit smaller so merging will be less painful.

Do we have a spec for this feature? As I originally understood it this was the ability to redirect our download attempts somewhere else; this "crawl" attempt thing is something very different. I don't know if that changed from when I looked last or if I was just blind due to being delirious trying to ship jthread in STL land.

@@ -154,6 +154,10 @@ namespace vcpkg
constexpr static StringLiteral WAIT_FOR_LOCK_SWITCH = "x-wait-for-lock";
Optional<bool> wait_for_lock = nullopt;

constexpr static StringLiteral VCPKG_EXPERIMENTAL_MIRROR_URL = "VCPKG_EXPERIMENTAL_MIRROR_URL";
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should name the variable EXPERIMENTAL because someday it won't be and there's no reason to force everyone to edit their scripts then?

Copy link
Contributor

Choose a reason for hiding this comment

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

OTOH we can continue respecting the experimental form for backwards compatibility and this makes it clear for users that they are using an unsupported feature. Alternatively, the tool could emit a big yelllow warning at the top every time it's run with this env var set.

toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.mirror.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.mirror.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.mirror.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.cpp Outdated Show resolved Hide resolved
@@ -154,6 +154,10 @@ namespace vcpkg
constexpr static StringLiteral WAIT_FOR_LOCK_SWITCH = "x-wait-for-lock";
Optional<bool> wait_for_lock = nullopt;

constexpr static StringLiteral VCPKG_EXPERIMENTAL_MIRROR_URL = "VCPKG_EXPERIMENTAL_MIRROR_URL";
Copy link
Contributor

Choose a reason for hiding this comment

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

We conventionally use lowercase here

Suggested change
constexpr static StringLiteral VCPKG_EXPERIMENTAL_MIRROR_URL = "VCPKG_EXPERIMENTAL_MIRROR_URL";
constexpr static StringLiteral VCPKG_EXPERIMENTAL_MIRROR_URL = "x-mirror-url";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The environment variable name is set here. Is this correct?

toolsrc/src/vcpkg/commands.mirror.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.mirror.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.mirror.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/downloads.cpp Outdated Show resolved Hide resolved
if (paths.vcpkg_use_mirror)
{
url = "ftp://";
url += paths.vcpkg_mirror_url;
Copy link
Contributor

Choose a reason for hiding this comment

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

If vcpkg_mirror_url is changed to include the protocol, this would enable https mirrors as well and generally be more robust

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, this should arguably be moved into download_file() so that every case across vcpkg uses it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am currently unable to change it to https, can we make these changes later?

toolsrc/include/vcpkg/commands.mirror.h Show resolved Hide resolved
toolsrc/include/vcpkg/base/downloads.h Outdated Show resolved Hide resolved
toolsrc/include/vcpkg/base/downloads.h Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor Author

Since the source package downloaded using x-mirror will be automatically renamed to sha512 value, do we need to provide the list of downloaded source packages to the mirror maintainer?

@JackBoosY JackBoosY requested a review from BillyONeal September 9, 2020 05:33
@JackBoosY
Copy link
Contributor Author

@ras0219 @ras0219-msft Please review again.

@soroshsabz
Copy link
Contributor

@JackBoosY why this PR closed? Where this functionality exist?

Thanks

@BillyONeal
Copy link
Member

@JackBoosY why this PR closed? Where this functionality exist?

Thanks

It needs to be replayed over in vcpkg-tool instead at a minimum

@JackBoosY
Copy link
Contributor Author

This PR was replaced by #13639.

@JackBoosY JackBoosY deleted the dev/jack/vcpkg_mirror branch May 26, 2022 06:47
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.

[vcpkg] Support mirror
5 participants