Skip to content

Download refactoring#4427

Merged
MikeMcQuaid merged 3 commits intoHomebrew:masterfrom
reitermarkus:download-refactoring
Jul 5, 2018
Merged

Download refactoring#4427
MikeMcQuaid merged 3 commits intoHomebrew:masterfrom
reitermarkus:download-refactoring

Conversation

@reitermarkus
Copy link
Copy Markdown
Member

@reitermarkus reitermarkus commented Jul 5, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Clean up some things related to download strategies.

@ghost ghost assigned reitermarkus Jul 5, 2018
@ghost ghost added the in progress Maintainers are working on this label Jul 5, 2018

def stage
super
quiet_safe_system "svn", "export", "--force", cached_location, Dir.pwd
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are these getting unquieted? In the previous PR you only unquieted those that had -q flags.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why are these getting unquieted?

These are not quiet currently. quiet_safe_system replaces i.e. { quiet_flag: '--quiet' }, which only cvs is using.

cached = downloader.cached_location.exist?
downloader.fetch
ohai "Pouring the cached bottle" if cached
@bottle_filename = downloader.cached_location
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the reordering about; does this change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This doesn't change anything at the moment, but this ordering is necessary for what I have in mind for refactoring the download locations.

Instead of relying on a somewhat hacky way to get the proper extension from a URL before downloading, we use no extension when initializing the download. We then know the actual file name after the download is finished, so in this case @bottle_filename should be set only after downloader.fetch has been called.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So currently a cancelled download would have a correct extension – whether guessed in a "hacky" manner or not – and after the change a cancelled download would be extensionless? That sounds like a regression to me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What use does a correct extension have when the download is cancelled?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most of the downloads can be resumed – please do not remove that ability as it's very useful – but foo-1.2.3.tar.bz2.incomplete shouldn't be resumable if the extension changed from bz2 to xz. Also it's much more clear what a foo-1.2.3.tar.bz2.incomplete is than what a foo-1.2.3.incomplete is.

@MikeMcQuaid
Copy link
Copy Markdown
Member

Thanks for explaining all the changes! Looks good to me.

@MikeMcQuaid MikeMcQuaid merged commit 4e70a42 into Homebrew:master Jul 5, 2018
@ghost ghost removed the in progress Maintainers are working on this label Jul 5, 2018
@reitermarkus reitermarkus deleted the download-refactoring branch July 6, 2018 08:12
@lock lock bot added the outdated PR was locked due to age label Aug 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants