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

Replace node downloader with vcpkg x-download. #826

Merged
merged 13 commits into from
Dec 10, 2022

Conversation

BillyONeal
Copy link
Member

Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1494956.
This will also make artifacts respect asset caching settings if a SHA512 is provided.

  • Calls vcpkg x-download and removes all attempts to download from within Node.
  • Adds a console parser for curl's output to extract percentages.
  • Adds progress tracking to WinHTTP.
  • x-download learns a new switch --z-machine-readable-progress, which artifacts reads to power its progress bars.
  • Downloader messages were localized.
  • WinHTTP efficiency improved slightly by not calling WinHttpQueryDataAvailable.
  • WinHTTP backend now correctly checks that the data is actually written to disk rather than assuming that fwrite succeeds.

@autoantwort
Copy link
Contributor

If you only want the progress in percentage you can use --progress-bar.

And

It's a risky business attempting to machine parse output that was never intended for machine consumption as the tools in question don't treat it as contractual. curls progress bars certainly qualify as "never intended for machine consumption" as far as I know.

@BillyONeal
Copy link
Member Author

It's a risky business attempting to machine parse output that was never intended for machine consumption as the tools in question don't treat it as contractual.

I did say that, and I still agree that is true. However, there's a very big difference in effect here. The worst thing that happens if the output changes and we parse the percentage incorrectly is funny ephemeral progress bar output. Nothing gets edited on disk or similar in the previous discussion where we were talking about mutating portfile.cmake.

Would it be better to check for the presence of those first 2 lines that are the 'key'? The specific order is not documented to be contractual but that k/M/G/etc are 1024s is, see https://curl.se/docs/manpage.html

Also asked curl's maintainer here: https://twitter.com/MalwareMinigun/status/1600570213572825090

If you only want the progress in percentage you can use --progress-bar.

It doesn't use --progress-bar explicitly because that is the "friendly human readable" version rather than the consumable by machines version.

curls progress bars certainly qualify as "never intended for machine consumption" as far as I know.

That's exactly why it doesn't use --progress-bar.

@BillyONeal
Copy link
Member Author

Daniel Stenberg clarified that it's at least semi-contractual here: https://twitter.com/bagder/status/1600615752725307400

image

# Conflicts:
#	include/vcpkg/base/messages.h
#	src/vcpkg/commands.xdownload.cpp
@BillyONeal BillyONeal merged commit 8350f07 into microsoft:main Dec 10, 2022
@BillyONeal BillyONeal deleted the download-vcpkg branch December 10, 2022 04:29
@BillyONeal
Copy link
Member Author

Thanks again so much @vicroms :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants