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

fix(scoop-download): Add failure check #4822

Merged
merged 3 commits into from
Mar 18, 2022

Conversation

tech189
Copy link
Member

@tech189 tech189 commented Mar 17, 2022

Description

  • When scoop-download catches an error in dl_with_cache, now a variable is saved so that the final success message will not appear.
  • When aria2 is enabled but not installed, Test-Aria2Enabled now warns the user that the standard download method will be used instead of aria2

Motivation and Context

Closes #4810

How Has This Been Tested?

I tested running this without internet and the failure was successfully caught, I tested both with aria2 and without so I have tested dl_with_cache and dl_with_cache_aria2.

This shows both changes in action:

❯ scoop config aria2-enabled
True

~
❯ c:\Users\me\Documents\GitHub\Scoop\libexec\scoop-download.ps1 protobuf
INFO  Starting download for protobuf...
WARN  aria2 is enabled but not installed, using standard download instead.
No such host is known. (github.com:443)
ERROR URL https://github.com/google/protobuf/releases/download/v3.19.4/protoc-3.19.4-win64.zip is not valid

^^ see no "success" and a warning about aria2!

Checklist:

  • I have read the Contributing Guide.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

^^ is there any documentation I should add and are there any tests to update?

@rashil2000
Copy link
Member

When aria2 is enabled but not installed, Test-Aria2Enabled now warns the user that the standard download method will be used instead of aria2

I don't think we should do this. The default value of aria2-enabled config variable is always true, meaning that the warning will always be shown to users who are new to Scoop, and users who have never installed aria2. This will be annoying for some people, and is a breaking UX change.

@tech189
Copy link
Member Author

tech189 commented Mar 17, 2022

Oh, I didn't know that aria2-enabled is set to true by default for new users. Why is it set to true if aria2 is not installed by default?

@rashil2000
Copy link
Member

Why is it set to true if aria2 is not installed by default?

See https://github.com/ScoopInstaller/Scoop#multi-connection-downloads-with-aria2

Aria2 was supposed to provide a better (and resumable) download experience, so the devs enabled it by default. The only requirement was installing aria2

@tech189
Copy link
Member Author

tech189 commented Mar 17, 2022

Huh weird, kinda defeats the point of having a config if it's on by default but doing nothing. But anyway yeah I'll get rid of that warning/check.

@rashil2000 rashil2000 changed the title scoop-download failure check, aria2 warn if uninstalled fix(scoop-download): Add failure check Mar 18, 2022
@rashil2000
Copy link
Member

LGTM. Please update the changelog (with same entry as the PR's title).

@tech189
Copy link
Member Author

tech189 commented Mar 18, 2022

Done! Are there any tests to run?

@rashil2000
Copy link
Member

No

@rashil2000 rashil2000 merged commit 53cdf68 into ScoopInstaller:develop Mar 18, 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