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

Add a "status" API to DiagnosticContext, overhaul Downloads console output #1565

Merged
merged 28 commits into from
Jan 15, 2025

Conversation

BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Jan 8, 2025

Extensive overhaul of our downloads handling and console output; @JavierMatosD and I have gone back and forth several times and yet kept introducing unintended bugs in other places, which led me to believe targeted fixes would no longer cut it.

Fixes many longstanding bugs and hopefully makes our console output for this more understandable:

  • We no longer print 'error' when an asset cache misses but the authoritative download succeeds. This partially undoes Editing asset cache output when using x-script #1541. It is good to print errors immediately when they happen, but if a subsequent authoritative download succeeds we need to not print those errors.
  • We now always and consistently print output from x-script s at the time that actually happens. Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2300063
  • We don't tell the user that proxy settings might fix a hash mismatch problem.
  • We do tell the user that proxy settings might fix a download from asset cache problem.
  • We now always tell the user the full command line we tried when invoking an x-script that fails.
  • We don't crash if an x-script doesn't create the file we expect, or creates a file with the wrong hash.
  • We now always print what we are doing before touching the network, so if we hang the user knows which server is being problematic. Note that this includes storing back to asset caches which we were previously entirely silent about except in case of failure.

Other changes:

  • Removed debug output about asset cache configuration. The output was misleading / wrong depending on readwrite settings, and echoing to the user exactly what they said before we've interpreted it is not useful debug output. (Contrast with other VcpkgPaths debug output which tend to be paths we have likely changed from something a user said)

Other notes:

  • This makes all dependencies of Binary cache: async push_success #908 speak DiagnosticContext so it will be easy to audit that the foreground/background thread behavior is correct after this.
  • I did test the curl status parsing on old Ubuntu again.

Special thanks to @JavierMatosD for his help in review of the first console output attempts and for blowing the dust off this area in the first place.

…or information, and change all MessageSink interaction to be line-oriented.

More than one line can be printed at a time, but there must be a line *between* messages.
…on rather than saying there was a download failure when no download was attempted.
Fixes many longstanding bugs and hopefully makes our console output for this more understandable:
* We no longer print 'error' when an asset cache misses but the authoritative download succeeds.
* We now always and consistently print output from x-script s at the time that actually happens. Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2300063
* We don't tell the user that proxy settings might fix a hash mismatch problem.
* We do the the user that proxy settings might fix a download from asset cache problem.
* We now always tell the user the full command line we tried when invoking an x-script that fails.
* We don't crash if an x-script doesn't create the file we expect, or creates a file with the wrong hash.
* We now always print what we are doing *before* touching the network, so if we hang the user knows which server is being problematic. Note that this includes storing back to asset caches which we were previously entirely silent about except in case of failure.

Other changes:
* Removed debug output about asset cache configuration. The output was misleading / wrong depending on readwrite settings, and echoing to the user exactly what they said before we've interpreted it is not useful debug output. (Contrast with other VcpkgPaths debug output which tend to be paths we have likely changed from something a user said)
Copy link
Contributor

@JavierMatosD JavierMatosD left a comment

Choose a reason for hiding this comment

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

Partial Review: I just reviewed the output based on the end to end tests. What happens in a sha mismatch scenario with x-block-origin enabled?

#azurl (no), x-block-origin (no), asset-cache (n/a), download (sha-mismatch)
#Expected: Download message with the "you might need to configure a proxy" message and with expected/actual sha
Refresh-TestRoot
$expected = @(
"Downloading https://example\.com -> example3\.html",
"[^\n]+example3\.html\.\d+\.part: error: download from https://example\.com had an unexpected hash",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want the error:?

Suggested change
"[^\n]+example3\.html\.\d+\.part: error: download from https://example\.com had an unexpected hash",
"[^\n]+example3\.html\.\d+\.part: warning: download from https://example\.com had an unexpected hash",

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fatal and we can't continue after it happens, so I think it needs to be error?

Copy link
Contributor

@JavierMatosD JavierMatosD Jan 9, 2025

Choose a reason for hiding this comment

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

can't continue after it happens

I thought sha mismatch is no longer fatal? We will turn around and try the authoritative source anyways.

Copy link
Member Author

@BillyONeal BillyONeal Jan 9, 2025

Choose a reason for hiding this comment

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

No, SHA mismatch immediately dies. We do not try anything else. (We just print the expected/actual hash message and treat it as a download failure rather than immediately terminating vcpkg.exe)

Copy link
Member Author

Choose a reason for hiding this comment

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

We did decide to make this an ordinary download failure rather than a fatal error, but this remains 'error'. In the cases where it is like a warning, we print nothing because the overall download succeeded. The error is only printed if there's no authoritative source that can do it either.

azure-pipelines/end-to-end-tests-dir/asset-caching.ps1 Outdated Show resolved Hide resolved
include/vcpkg/base/fwd/downloads.h Outdated Show resolved Hide resolved
include/vcpkg/base/hash.h Outdated Show resolved Hide resolved
include/vcpkg/base/system.process.h Show resolved Hide resolved
src/vcpkg/binarycaching.cpp Show resolved Hide resolved
src/vcpkg/base/downloads.cpp Outdated Show resolved Hide resolved
src/vcpkg/base/downloads.cpp Outdated Show resolved Hide resolved
@BillyONeal BillyONeal marked this pull request as ready for review January 9, 2025 21:51
@BillyONeal BillyONeal merged commit e70b618 into microsoft:main Jan 15, 2025
6 checks passed
@BillyONeal BillyONeal deleted the message-sink-line branch January 15, 2025 01:15
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.

2 participants