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

feat(install): Follow HTTP redirections when downloading a file #3902

Merged
merged 3 commits into from
Apr 20, 2020

Conversation

tynril
Copy link
Contributor

@tynril tynril commented Mar 29, 2020

In some cases, such as when an HTTPS URL redirects to an HTTP URL, the HTTPWebRequest client may not follow 302 error codes. This change explicitly catches that error, and attempts to find another URL for the same file in the body of the 302 response.

This specifically fixes the error when installing or updating Krita (from the extras bucket), which has an HTTPS download URL that can redirect to HTTP mirrors.

Closes #3886
Closes #3535
Closes ScoopInstaller/Extras#2728
Closes ScoopInstaller/Extras#2932
Replaces ScoopInstaller/Extras#2933

Copy link
Contributor

@Ash258 Ash258 left a comment

Choose a reason for hiding this comment

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

  • Rebase to develop
  • Use proper casing for methods, properties

lib/install.ps1 Outdated Show resolved Hide resolved
lib/install.ps1 Outdated Show resolved Hide resolved
@Ash258
Copy link
Contributor

Ash258 commented Mar 29, 2020

Looks really good. I will test it on other manifests than krita.

image

@tynril tynril changed the base branch from master to develop March 29, 2020 21:11
lib/install.ps1 Outdated Show resolved Hide resolved
lib/install.ps1 Outdated Show resolved Hide resolved
In some cases, such as when an HTTPS URL redirects to an HTTP URL, the
`HTTPWebRequest` client may not follow 302 error codes. This change explicitly
catches that error, and attempts to find another URL for the same file in the
body of the 302 response.

This specifically fixes the error when installing or updating Krita (from
the extras bucket), which has an HTTPS download URL that can redirect to HTTP
mirrors.
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
@tynril
Copy link
Contributor Author

tynril commented Mar 29, 2020

Thanks for the reviews! I cleaned the history a bit, let me know if you'd like me to squash one last time once this is finished.

As far as testing goes, in addition to Krita, I've tried this change with java/oraclejdk as well (the one described in #3886). There might be more.

@Ash258
Copy link
Contributor

Ash258 commented Mar 29, 2020

Basically any of the apache applications are affected by this. I will look for some other.

@se35710
Copy link
Contributor

se35710 commented Mar 30, 2020

@Ash258 Once in master, we could revert the download urls for all the Eclipse manifests in extras that were changed in commit ScoopInstaller/Extras@2ec0e54

This will provide the Eclipse project with better statistics, and scoop users with better download speeds.

lib/install.ps1 Outdated Show resolved Hide resolved
@Ash258
Copy link
Contributor

Ash258 commented Mar 30, 2020

@se35710 Sure. I will then handle it for extras as there will be much more manifests, which could benefit from this. I also need to look for all opened issues across all repositories to close then in bulk.

@tynril tynril changed the title Follow 302 Redirect when downloading a file Follow HTTP redirections when downloading a file Mar 30, 2020
Copy link
Member

@chawyehsu chawyehsu left a comment

Choose a reason for hiding this comment

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

works great for my 301/307 cases.

@chawyehsu
Copy link
Member

So this is ready to merge I think. @r15ch13

@r15ch13 r15ch13 changed the title Follow HTTP redirections when downloading a file feat(install): Follow HTTP redirections when downloading a file Apr 20, 2020
@r15ch13 r15ch13 merged commit e6b355e into ScoopInstaller:develop Apr 20, 2020
@iguyking
Copy link

@Ash258 Thanks for finding the related issue. If this has been merged, why am I still seeing the issue?

@jat001
Copy link

jat001 commented May 25, 2020

@Ash258 Thanks for finding the related issue. If this has been merged, why am I still seeing the issue?

You have to change the branch of scoop to develop. This PR has not merged into master yet.

@iguyking
Copy link

@jat001 Where? Do I reinstall scoop directly to shift to develop branch? Is there a Wiki page I can read or a contributing.md that I'm missing?

@jat001
Copy link

jat001 commented May 26, 2020

@jat001 Where? Do I reinstall scoop directly to shift to develop branch? Is there a Wiki page I can read or a contributing.md that I'm missing?

well, you are not familiar with git, right?

open %UserProfile%\scoop\apps\scoop\current\.git\config
change fetch = +refs/heads/master:refs/remotes/origin/master to fetch = +refs/heads/*:refs/remotes/origin/*

in terminal
cd %UserProfile%\scoop\apps\scoop\current
git fetch --all
git checkout -b develop

@iguyking
Copy link

Very familiar with git. Just wasn't sure the approach to being able to contribute. Thanks.

@linsui
Copy link
Contributor

linsui commented May 26, 2020

You can just use scoop config SCOOP_BRANCH develop

@iguyking
Copy link

That's awesome. Is there a way to specify a branch for testing of the buckets? (if there is somewhere more appropriate for this conversation say a discord or mailing list let me know) I was digging into trying to improve some of the other deploys and didn't see any way to do that. The way I think is to fork the bucket and then test directly in the forked versions master branch.

@linsui
Copy link
Contributor

linsui commented May 26, 2020

What do you want to test? Scoop core or some manifests? You can open another issue or join https://discord.gg/s9yRQHt 😄

@AntonOks
Copy link

AntonOks commented Sep 8, 2020

@Ash258 Thanks for finding the related issue. If this has been merged, why am I still seeing the issue?

You have to change the branch of scoop to develop. This PR has not merged into master yet.

Any timeline anyone for an upcoming / planed merge?

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