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

[aria2] use env variable to use aria2 download tool #18171

Closed
wants to merge 2 commits into from

Conversation

cenit
Copy link
Contributor

@cenit cenit commented May 28, 2021

Describe the pull request

@JackBoosY it seems easier than expected.
I added a VCPKG_DOWNLOAD_TOOL env variable symmetric to the already-in-use _VCPKG_DOWNLOAD_TOOL managed by the cli switch --x-use-aria2

@JackBoosY JackBoosY self-assigned this May 28, 2021
@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label May 28, 2021
@JackBoosY JackBoosY requested a review from strega-nil-ms May 28, 2021 08:43
@JackBoosY
Copy link
Contributor

Okay, I will take over this PR.

@cenit
Copy link
Contributor Author

cenit commented May 28, 2021

it might require also cenit/vcpkg-tool@2364e22

I can open a PR also on vcpkg-tool if it's the right way to proceed with this problem

@cenit cenit force-pushed the dev/cenit/aria2 branch from 9a9fbfd to 572a926 Compare May 28, 2021 12:17
@cenit
Copy link
Contributor Author

cenit commented May 28, 2021

it works locally (together with the PR on the vcpkg-tool microsoft/vcpkg-tool#85), I hope the same is valid for anyone willing to try.

I am sorry for the full rebuild that this PR is triggering, but I hope for this PR to be considered for merging. The full rebuild is totally unnecessary because it's using the "old" vcpkg-tool and in any case would not have the env variable defined even using the newer one

main reason for this PR: I don't know why, but cmake built-in download is really very broken on windows behind proxy, while at the same time aria2 always works flawlessy

@cenit
Copy link
Contributor Author

cenit commented May 28, 2021

@strega-nil @strega-nil-ms please let me know if the modification is not coherent with the idea behind env variables. To pilot vcpkg in manifest mode they are almost the only way that I know

@cenit
Copy link
Contributor Author

cenit commented Jun 4, 2021

@JackBoosY any news here (and vcpkg-tool related PR)? it might be very useful for people behind corporate firewalls!

@JackBoosY
Copy link
Contributor

Oh sorry, last week I'm working on #18201.
Will handle this PR this week.

@cenit
Copy link
Contributor Author

cenit commented Jun 7, 2021

merged with master just to be sure that the recent #13639 was not interfering in the logic.
It seems that it's still ok without any required modification @JackBoosY

@ras0219-msft
Copy link
Contributor

We'd really like to see the aria functionality moved into the vcpkg tool and remove the remaining workaround in vcpkg_download_distfile(). That way it will be applied uniformly even for our other downloads that are performed inside vcpkg.exe.

@JackBoosY
Copy link
Contributor

@ras0219-msft Can you describe it in more detail?
I think that even if aria2 is used to download files uniformly, vcpkg_download_dstfile should exist as a wrapper.

@cenit
Copy link
Contributor Author

cenit commented Jun 8, 2021

We'd really like to see the aria functionality moved into the vcpkg tool and remove the remaining workaround in vcpkg_download_distfile(). That way it will be applied uniformly even for our other downloads that are performed inside vcpkg.exe.

with or without using libaria2? https://github.com/aria2/aria2#libaria2
To be honest, I really like unix philosophy of keeping it simple. So why should vcpkg also become a download tool? I understand that x-download is already in, but why?

@cenit
Copy link
Contributor Author

cenit commented Jun 8, 2021

also, is this PR even considered for now or would you prefer a full solution immediately with aria/aria-like download integrated into vcpkg?
I understand that this (imho long due) env variable can offer a quick solution, but I understand that I am trying to "quick solve" a problem which I also do not fully understand. If you ask me why aria2 (or browser) succeeds while cmake or vcpkg fail at download the same file on the same node in the same network, well I don't know

@BillyONeal
Copy link
Member

This is not correct because you've bypassed the part that acquires aria2 in the first place; I think if you want to do this it really does need to be in the tool proper (and it needs to be validity checked instead of an open ended string like this... the current leading underscore form is OK because we know where it's being set...)

@ras0219
Copy link
Contributor

ras0219 commented Jun 14, 2021

So why should vcpkg also become a download tool?

The idea is not that vcpkg should implement HTTP, but rather that:

  1. vcpkg itself must download things (cmake, ninja, git, binarycaching, etc)
  2. portfile.cmake needs to download things (sources, buildsystems)
  3. We want to be able to uniformly customize and control downloading (source mirroring, authentication, proxies)

Therefore, it makes sense that vcpkg is the uniform entry point for all downloading; it then internally can dispatch to whatever mechanism is appropriate (WinHTTP, curl, aria2).

I'd prefer to avoid this half-solution, since we know we need to move all download functionality into the tool very soon.

@cenit
Copy link
Contributor Author

cenit commented Jun 15, 2021

ok, understood. Closing the PR, and looking forward to seeing the new unified architecture :)

@cenit cenit closed this Jun 15, 2021
@cenit cenit deleted the dev/cenit/aria2 branch February 14, 2025 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aria2] use environment variable to select download tool
5 participants