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_download_distfile] Fix the parameter mismatch issues of some internal functions and fix the recursive call (step 3) #20585

Merged
merged 12 commits into from
Nov 26, 2021

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Oct 8, 2021

  1. In the PR [rollup:2021-08-09] Rollup PR #19469, the modification of some parameters of z_vcpkg_download_distfile_test_hash has not been completed, and caused an error when using aira2 to verify the hash:
Starting package 1/1001: abseil:x64-linux
Building package abseil[core]:x64-linux...
-- Downloading abseil-abseil-cpp-278e0a071885a22dcd2fd1b5576cc44757299343.tar.gz...
CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:139 (z_vcpkg_download_distfile_test_hash):
  z_vcpkg_download_distfile_test_hash Function invoked with incorrect
  arguments for function named: z_vcpkg_download_distfile_test_hash
Call Stack (most recent call first):
  scripts/cmake/vcpkg_download_distfile.cmake:227 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_from_github.cmake:173 (vcpkg_download_distfile)
  ports/abseil/portfile.cmake:5 (vcpkg_from_github)
  scripts/ports.cmake:141 (include)


Downloaded sources for package abseil[core]:x64-linux
  1. Fix recursive calls when using extra option --x-use-aria2:
tarting package 3/164: boost-config:x86-windows
Building package boost-config[core]:x86-windows...
CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program):
  Maximum recursion depth of 1000 exceeded
Call Stack (most recent call first):
  scripts/cmake/vcpkg_download_distfile.cmake:231 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_find_acquire_program.cmake:552 (vcpkg_download_distfile)
  scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program)
  scripts/cmake/vcpkg_download_distfile.cmake:231 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_find_acquire_program.cmake:552 (vcpkg_download_distfile)
  scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program)
  scripts/cmake/vcpkg_download_distfile.cmake:231 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_find_acquire_program.cmake:552 (vcpkg_download_distfile)
  scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program)
  scripts/cmake/vcpkg_download_distfile.cmake:231 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_find_acquire_program.cmake:552 (vcpkg_download_distfile)
  scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program)

@JackBoosY JackBoosY added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:internal This PR or Issue was filed by the vcpkg team. labels Oct 8, 2021
@JackBoosY JackBoosY marked this pull request as draft October 8, 2021 07:28
@JackBoosY
Copy link
Contributor Author

JackBoosY commented Oct 8, 2021

@strega-nil-ms
It looks like we have a problem with recursive calls (use aria2 in download process):

tarting package 3/164: boost-config:x86-windows
Building package boost-config[core]:x86-windows...
CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program):
  Maximum recursion depth of 1000 exceeded
Call Stack (most recent call first):
  scripts/cmake/vcpkg_download_distfile.cmake:231 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_find_acquire_program.cmake:552 (vcpkg_download_distfile)
  scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program)
  scripts/cmake/vcpkg_download_distfile.cmake:231 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_find_acquire_program.cmake:552 (vcpkg_download_distfile)
  scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program)
  scripts/cmake/vcpkg_download_distfile.cmake:231 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_find_acquire_program.cmake:552 (vcpkg_download_distfile)
  scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program)
  scripts/cmake/vcpkg_download_distfile.cmake:231 (z_vcpkg_download_distfile_via_aria)
  scripts/cmake/vcpkg_find_acquire_program.cmake:552 (vcpkg_download_distfile)
  scripts/cmake/vcpkg_download_distfile.cmake:110 (vcpkg_find_acquire_program)
...

z_vcpkg_download_distfile_via_aria requires ARIA2 using vcpkg_find_acquire_program(ARIA2)
vcpkg_find_acquire_program requires vcpkg_download_distfile
vcpkg_download_distfile calls z_vcpkg_download_distfile_via_aria

@JackBoosY JackBoosY changed the title [vcpkg_download_distfile] Fix incorrect arguments in z_vcpkg_download_distfile_test_hash [vcpkg_download_distfile] Fix the parameter mismatch issues of some internal functions and fix the recursive call Oct 8, 2021
@JackBoosY JackBoosY marked this pull request as ready for review October 8, 2021 08:56
@julianxhokaxhiu
Copy link
Contributor

One small note: it's ARIA2 not AIRA2.

Other than that, thanks, looking forward to it :)

@JackBoosY
Copy link
Contributor Author

Ping @strega-nil-ms for review this PR.

@PhoebeHui PhoebeHui added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 9, 2021
@Akarinnnnn
Copy link

Thank you, this works for #20648

@BillyONeal
Copy link
Member

Tagging requires:discussion to decide if we want to route fetching aria2 through vcpkg fetch since it behaves "logically" as a builtin like cmake or ninja here.

@JackBoosY
Copy link
Contributor Author

Fine, I will change aria2 as a vcpkg requires tools.

@JackBoosY JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Nov 3, 2021
@JackBoosY
Copy link
Contributor Author

I will test this changes in my private mirror tonight.

@JackBoosY
Copy link
Contributor Author

This PR also needs to update the vcpkg min version.

@JackBoosY
Copy link
Contributor Author

Checked with my private mirror: success.

@julianxhokaxhiu
Copy link
Contributor

Is the PR stuck?

@BillyONeal
Copy link
Member

Is the PR stuck?

No, but since it effectively triggers a world rebuild it's slow going because a build failure pretty much anywhere forces a lot of compute time so cycle time is long

@JackBoosY
Copy link
Contributor Author

Depends on #21344.

@JackBoosY JackBoosY added depends:different-pr This PR or Issue depends on a PR which has been filed and removed depends:different-pr This PR or Issue depends on a PR which has been filed labels Nov 15, 2021
@JackBoosY
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor Author

For tensorflow and tensorflow-cc regression:

ERROR: Failed cleaning build dir for numpy

@BillyONeal Any ideas to solve this?

@BillyONeal
Copy link
Member

Thanks for the fix for aria2. :)

@BillyONeal BillyONeal merged commit 520b5c4 into microsoft:master Nov 26, 2021
@JackBoosY JackBoosY deleted the dev/jack/20574 branch November 26, 2021 07:22
@julianxhokaxhiu
Copy link
Contributor

Thank you! One more question: how are we supposed to use Aria2 now? I noticed the --x-use-aria2 flag is gone, is it now used by default?

@BillyONeal
Copy link
Member

Thank you! One more question: how are we supposed to use Aria2 now? I noticed the --x-use-aria2 flag is gone, is it now used by default?

It shouldn't be gone, but we did remove some x- switches from help text recently in order to reduce console spew.

@julianxhokaxhiu
Copy link
Contributor

julianxhokaxhiu commented Nov 28, 2021

It shouldn't be gone, but we did remove some x- switches from help text recently in order to reduce console spew.

Oh I see thanks, where can I find those flags if not printed on the console? Just to know for the future :) Thanks!

//EDIT: Nvm, found it! https://github.com/microsoft/vcpkg-tool/blob/main/src/vcpkg/install.cpp#L795

@BillyONeal
Copy link
Member

Just be aware that -x'd things can go away :)

@julianxhokaxhiu
Copy link
Contributor

Just be aware that -x'd things can go away :)

Is there a long term flag I can rely upon then? Maybe via CMake?

@BillyONeal
Copy link
Member

Not for this aria2 setting. My limited understanding is that it was x-'d because we don't really have a concrete case for it to be a strongly supported option. Someone contributed it, and as long as we have no reason to break it, we aren't going to. But we also aren't testing it etc. like non x-'d scenarios, resulting in bugs like the one that spawned this PR in the first place.

That aria2's licensing isn't compatible with ours so we can't distribute together, and we have this catch22 problem of needing to use something else to download it in the first place, remains kind of an issue here.

@julianxhokaxhiu
Copy link
Contributor

That aria2's licensing isn't compatible with ours so we can't distribute together, and we have this catch22 problem of needing to use something else to download it in the first place, remains kind of an issue here.

Do you think it's possible to open a discussion with ARIA2 author somehow to dual license the project to fit the needs of vcpkg?

@BillyONeal
Copy link
Member

No idea. Either way it would depend on a better understanding of why someone is attempting to replace it with aria2 in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:internal This PR or Issue was filed by the vcpkg team. requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
5 participants