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

Workflow improvements to increase winget command success rate #648

Merged
merged 10 commits into from
Nov 26, 2020

Conversation

yao-msft
Copy link
Contributor

@yao-msft yao-msft commented Nov 21, 2020

Change

  • Check IAttachementExecute:Save() result instead of ignoring and give better output message
  • Make source update failure not blocking during source open for winget install, upgrade, etc
  • Make the synchronization lock accept WAIT_ABANDON since we always write everything back
  • In downloader.cpp, check download size matches content-length to detect corrupt downloads if possible
  • Retry for installer downloads and manifest downloads
  • Make ERROR_NO_RANGES_PROCESSED error more specific when connection failed

Validation

Existing tests. Manual verification by installing several apps succeeded.

Microsoft Reviewers: Open in CodeFlow

@yao-msft yao-msft requested a review from a team as a code owner November 21, 2020 05:51
@@ -135,7 +164,8 @@ namespace AppInstaller::CLI::Workflow
}
catch (const winrt::hresult_error& e)
{
if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED))
if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED) ||
HRESULT_FACILITY(e.code()) == FACILITY_HTTP)
{
// Server does not support range request, use download
Copy link
Contributor

@ranm-msft ranm-msft Nov 26, 2020

Choose a reason for hiding this comment

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

support range request [](start = 35, length = 21)

update comment #Resolved

auto result = context.Reporter.ExecuteWithProgress(std::bind(Repository::OpenSource, sourceName, std::placeholders::_1), true);
source = result.Source;

for (const auto& s : result.SourcesWithUpdateFailure)
Copy link
Contributor

@ranm-msft ranm-msft Nov 26, 2020

Choose a reason for hiding this comment

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

for [](start = 12, length = 3)

add a comment indicating that this is only going to produce a warning but we'll continue with the operation (install/update etc.) #Resolved

// Check download size matches if content length is provided in response header
if (contentLength > 0)
{
THROW_HR_IF(E_UNEXPECTED, bytesDownloaded != contentLength);
Copy link
Contributor

@ranm-msft ranm-msft Nov 26, 2020

Choose a reason for hiding this comment

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

E_UNEXPECTED [](start = 24, length = 12)

let's create a new hresult reflecting the actual error of size mismatch
DOWNLOAD_SIZE_MISMATCH #Resolved

@@ -229,6 +237,8 @@ namespace AppInstaller::Utility

aesThread.join();

AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr);
AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr << " AES Save result: " << aesSaveResult);
Copy link
Contributor

@ranm-msft ranm-msft Nov 26, 2020

Choose a reason for hiding this comment

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

AES [](start = 104, length = 3)

let's say "Attachment Execution Service" #Resolved

Copy link
Contributor

@ranm-msft ranm-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@yao-msft yao-msft merged commit 92b3e73 into microsoft:master Nov 26, 2020
@yao-msft yao-msft deleted the improvesuccessrates branch November 26, 2020 03:29
jontab pushed a commit to jontab/winget-cli that referenced this pull request Jul 12, 2022
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