Separate staging from download. #4402

reitermarkus · 2018-07-01T21:39:18Z

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?

Separate unpacking the download from download strategies. These unpack strategies should then also used in the Container classes used for staging casks.

MikeMcQuaid

As a general comment it's not clear due to the file change what's been moved as-is and what's been changed. I'd suggest moving to unpack strategy in one PR (or at least one commit) and then making the relevant changes in another.

MikeMcQuaid · 2018-07-02T09:26:11Z

Library/Homebrew/unpack_strategy.rb

+    magic_number = magic_number(path)
+
+    strategies = [
+      JarUnpackStrategy,


Can this be inferred somehow rather than hardcoded? If not, a class level constant feels a bit nicer.

a class level constant feels a bit nicer.

Not sure how a constant could replace this.

A frozen constant array of classes rather than a variable.

Ah, I thought you were talking about JarUnpackStrategy.

MikeMcQuaid · 2018-07-02T09:26:40Z

Library/Homebrew/unpack_strategy.rb

+  def self.from(download:); end
+
+  def self.magic_number(path)
+    File.binread(path, 252)


A constant explaining what 252 means would be good.

MikeMcQuaid · 2018-07-02T09:27:10Z

Library/Homebrew/unpack_strategy.rb

+
+    # This is so that bad tarballs and zips produce good error messages.
+    strategy ||= case path.extname
+    when ".tar.gz", ".tgz", ".tar.bz2", ".tbz"


Might want to add .tar.xz here?

MikeMcQuaid · 2018-07-02T09:27:47Z

Library/Homebrew/unpack_strategy.rb

+  end
+
+  attr_reader :path, :magic_number
+  private :magic_number


I think it's probably nicer to just use the instance variable.

MikeMcQuaid · 2018-07-02T09:30:21Z

Library/Homebrew/unpack_strategy.rb

+      end
+    end
+  end
+  private :pipe_to_tar


Can you order these so you can have a single private?

I'm trying to follow the new RuboCop rule (which we disabled for now because of false positives). Basically, the private should be next to the method, so it is much easier to tell if a method is private or not.

Does that rule have an autocorrect? If not I think it's going to prove too painful to implement this consistently across the codebase (and I think remaining consistent for now would be better).

MikeMcQuaid · 2018-07-02T09:30:44Z

Library/Homebrew/unpack_strategy.rb

+end
+
+class UncompressedUnpackStrategy < UnpackStrategy
+  def self.can_extract?(**)


Would be nicer to avoid ** here I think.

MikeMcQuaid · 2018-07-02T09:31:25Z

Library/Homebrew/unpack_strategy.rb

+  end
+
+  def extract_to_dir(unpack_dir)
+    if OS.mac?


This should use os/extend (if it's needed, I'm not sure why it is)

MikeMcQuaid · 2018-07-02T09:31:35Z

Library/Homebrew/unpack_strategy.rb

+    return true if magic_number.match?(/^.{257}ustar/n)
+
+    # Check if `tar` can list the contents, then it can also extract it.
+    IO.popen(["tar", "-t", "-f", path], err: File::NULL) { |stdout| !stdout.read(1).nil? }


do form block here

reitermarkus · 2018-07-03T23:49:27Z

As a general comment it's not clear due to the file change what's been moved as-is and what's been changed. I'd suggest moving to unpack strategy in one PR (or at least one commit) and then making the relevant changes in another.

Nothing has been moved as-is, because previously everything was just unpacked in PWD. So every command had to be adapted to support unpacking into a specified directory.

MikeMcQuaid · 2018-07-04T08:48:58Z

Nothing has been moved as-is, because previously everything was just unpacked in PWD. So every command had to be adapted to support unpacking into a specified directory.

Sure; I'm suggesting this adaption happen in a future PR (or even separate commits in this one) and this one focuses on trying to just move things as-is. It's very difficult to review what's changed and what hasn't, as-is.

MikeMcQuaid · 2018-07-06T17:23:02Z

Library/Homebrew/download_strategy.rb

@@ -45,7 +46,9 @@ def ohai(*args)
  # Unpack {#cached_location} into the current working directory, and possibly
  # chdir into the newly-unpacked directory.
  # Unlike {Resource#stage}, this does not take a block.
-  def stage; end
+  def stage
+    UnpackStrategy.detect(cached_location).extract(basename: basename_without_params)


Stick .extract on a new line, maybe?

MikeMcQuaid · 2018-07-06T17:23:05Z

Library/Homebrew/download_strategy.rb

@@ -384,7 +344,7 @@ def _fetch
 # Useful for installing jars.
 class NoUnzipCurlDownloadStrategy < CurlDownloadStrategy
  def stage
-    cp cached_location, basename_without_params, preserve: true
+    UncompressedUnpackStrategy.new(cached_location).extract(basename: basename_without_params)


Stick .extract on a new line, maybe?

MikeMcQuaid · 2018-07-06T17:23:44Z

Library/Homebrew/download_strategy.rb

-      quiet_safe_system cvspath, { quiet_flag: "-Q" }, "-d", @url, "login" if @url.include? "pserver"
-      quiet_safe_system cvspath, { quiet_flag: "-Q" }, "-d", @url, "checkout", "-d", cache_filename, @module
+      quiet_safe_system "cvs", { quiet_flag: "-Q" }, "-d", @url, "login" if @url.include? "pserver"
+      quiet_safe_system "cvs", { quiet_flag: "-Q" }, "-d", @url, "checkout", "-d", cached_location.basename, @module, chdir: cached_location.dirname


Can this line be split across a few? It's pretty long.

MikeMcQuaid · 2018-07-06T17:24:51Z

Library/Homebrew/extend/os/mac/unpack_strategy.rb

@@ -0,0 +1,5 @@
+class ZipUnpackStrategy
+  def extract_to_dir(unpack_dir, **)
+    safe_system "ditto", "-x", "-k", "--", path, unpack_dir


What makes this preferable to unzip?

This keeps Finder attributes intact.

What's that needed for?

MikeMcQuaid · 2018-07-06T17:26:59Z

Library/Homebrew/unpack_strategy.rb

+      end
+    end
+  end
+  private :pipe_to_tar


Does that rule have an autocorrect? If not I think it's going to prove too painful to implement this consistently across the codebase (and I think remaining consistent for now would be better).

MikeMcQuaid · 2018-07-06T17:28:25Z

Library/Homebrew/unpack_strategy.rb

+    path.directory?
+  end
+
+  def extract_to_dir(unpack_dir, **)


Would be nicer to avoid ** here I think.

MikeMcQuaid · 2018-07-06T17:28:49Z

Library/Homebrew/unpack_strategy.rb

+end
+
+class P7ZipUnpackStrategy < UnpackStrategy
+  def self.can_extract?(magic_number:, **)


Would be nicer to avoid ** here I think (and everywhere below).

How would you avoid these?

Specify the arguments that would be passed.

Then RuboCop will complain about unused arguments.

@reitermarkus They can be prefixed with _ and there may be other options depending on the types of arguments (I could be wrong on this, though). I think that's preferable to accepting literally any number of random arguments.

Ok, there is a RuboCop setting to allow unused keyword arguments.

MikeMcQuaid · 2018-07-06T17:29:43Z

Library/Homebrew/unpack_strategy.rb

+    return true if magic_number.match?(/\A.{257}ustar/n)
+
+    # Check if `tar` can list the contents, then it can also extract it.
+    IO.popen(["tar", "-t", "-f", path], err: File::NULL) do |stdout|


Let's go back to tf here for consistency

MikeMcQuaid · 2018-07-06T17:31:01Z

Library/Homebrew/unpack_strategy.rb

+
+class ZipUnpackStrategy < UnpackStrategy
+  def self.can_extract?(magic_number:, **)
+    magic_number.match?(/\APK(\003\004|\005\006)/n)


Feels like there's maybe a nicer interface for this given pretty much all the self.can_extract methods are the same?

I can't think of any, since some need more than just the magic number.

How about the default implementation in UnpackStrategy being magic_number.match?(magic_regex) or something and each subclass can define magic_regex?

Tried it, but it only get's worse.

For example, XzUnpackStrategy < TarUnpackStrategy < FileUnpackStrategy:

This needs to override the custom can_extract? in TarUnpackStrategy, so it will be exactly the same as in FileUnpackStrategy again, and we also have to add a magic_regex constant/method.

Cool, no worries, thanks for checking.

MikeMcQuaid · 2018-07-06T17:31:25Z

Library/Homebrew/unpack_strategy.rb

+    return false unless magic_number.match?(/\ASQLite format 3\000/n)
+
+    # Fossil database is made up of artifacts, so the `artifact` table must exist.
+    Utils.popen_read("sqlite3", path, "select count(*) from sqlite_master where type = 'view' and name = 'artifact'").to_i == 1


This is a long line; can it get split?

MikeMcQuaid · 2018-07-08T13:55:27Z

Library/Homebrew/unpack_strategy.rb

+    path.directory?
+  end
+
+  def extract_to_dir(unpack_dir, **)


Keyword args look good; can this have the final args named _? Thanks.

MikeMcQuaid · 2018-07-08T13:56:00Z

Library/Homebrew/unpack_strategy.rb

+  def extract_to_dir(unpack_dir, **)
+    FileUtils.cp_r path.children, unpack_dir, preserve: true
+  end
+  private :extract_to_dir


Would still like to see this be consistent with the rest of Homebrew and, if you feel like it, a general fixup of the RuboCop rule after this is merged and consistent.

reitermarkus · 2018-07-11T05:08:54Z

@JCount and @MikeMcQuaid, one last look?

MikeMcQuaid

A few nits, none of which is blocking so feel free to disagree with any of them. Nice work 👍

MikeMcQuaid · 2018-07-12T08:22:34Z

Library/Homebrew/download_strategy.rb

-  def quiet_safe_system(*args)
-    safe_system(*expand_safe_system_args(args))
+  def basename_without_params
+    return nil if @url.nil?


return unless @url?

MikeMcQuaid · 2018-07-12T08:22:57Z

Library/Homebrew/download_strategy.rb

@@ -960,16 +886,23 @@ def repo_valid?
    (cached_location/"CVS").directory?
  end

+  def quiet_flag
+    ARGV.verbose? ? nil : "-Q"


"-Q" if ARGV.verbose??

MikeMcQuaid · 2018-07-12T08:25:19Z

Library/Homebrew/unpack_strategy.rb

+    magic_number = if path.directory?
+      ""
+    else
+      File.binread(path, MAX_MAGIC_NUMBER_LENGTH) || ""


Given the "" fallback in both cases I wonder if:

magic_number = File.binread(path, MAX_MAGIC_NUMBER_LENGTH) if path.directory? magic_number ||= ""

or similar might be better?

I tried some variations, but I like this version because it is more explicit.

MikeMcQuaid · 2018-07-12T08:29:11Z

Library/Homebrew/unpack_strategy.rb

+
+  def extract_nested_tar(unpack_dir, basename:)
+    return unless DependencyCollector.tar_needs_xz_dependency?
+    return unless (children = unpack_dir.children).count == 1


I have no real preference but if .. != sometimes reads nicer to me.

reitermarkus · 2018-07-12T09:23:35Z

Should be ready to go now.

JCount

Looks good to me, but let’s wait on @MikeMcQuaid before merging.

ilovezfs · 2018-07-18T07:35:28Z

iMac-TMP:Homebrew joe$ git bisect good
5b3bbb76c9d224c08ae498677868b69aab71ff7c is the first bad commit
commit 5b3bbb76c9d224c08ae498677868b69aab71ff7c
Author: Markus Reiter <[email protected]>
Date:   Sun Jul 1 23:35:29 2018 +0200

    Separate staging from download.

:040000 040000 6d5d52263444dbca74d030b9714bd87afff1d386 183e6d00db7777ddcadf1ce9017e320bed2d45c2 M	Library

This has caused some Git checkouts to be unclean that were previously clean.

For kubernetes-cli,

Before:

bash-3.2$ git status
Not currently on any branch.
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	.brew_home/

nothing added to commit but untracked files present (use "git add" to track)

After:

bash-3.2$ git status
Not currently on any branch.
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	typechange: .bazelrc
	typechange: .kazelcfg.json
	typechange: BUILD.bazel
	typechange: Makefile
	typechange: Makefile.generated_files
	typechange: WORKSPACE

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	.brew_home/

no changes added to commit (use "git add" and/or "git commit -a")

This is now causing CI failure for anything that checks that its build info reports a clean not dirty tag.

ilovezfs · 2018-07-20T06:29:58Z

iMac-TMP:Homebrew joe$ git bisect bad
5b3bbb76c9d224c08ae498677868b69aab71ff7c is the first bad commit
commit 5b3bbb76c9d224c08ae498677868b69aab71ff7c
Author: Markus Reiter <[email protected]>
Date:   Sun Jul 1 23:35:29 2018 +0200

    Separate staging from download.

:040000 040000 6d5d52263444dbca74d030b9714bd87afff1d386 183e6d00db7777ddcadf1ce9017e320bed2d45c2 M	Library
iMac-TMP:Homebrew joe$

This has broken the ability to install cromwell on El Capitan.

See https://jenkins.brew.sh/job/Homebrew%20Testing/1023/ and https://jenkins.brew.sh/job/Homebrew%20Core%20Pull%20Requests/27643/

==> brew install --verbose cromwell
==> FAILED
/usr/bin/sandbox-exec -f /private/tmp/homebrew20180720-32126-1i16pn6.sb nice /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/bin/ruby -W0 -I /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/gems/2.3.0/gems/did_you_mean-1.0.0/lib:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/site_ruby/2.3.0:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/site_ruby/2.3.0/x86_64-darwin9.0:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/site_ruby/2.3.0/universal-darwin9.0:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/site_ruby:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/vendor_ruby/2.3.0:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/vendor_ruby/2.3.0/x86_64-darwin9.0:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/vendor_ruby/2.3.0/universal-darwin9.0:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/vendor_ruby:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/2.3.0:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/2.3.0/x86_64-darwin9.0:/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/2.3.0/universal-darwin9.0:/usr/local/Homebrew/Library/Homebrew/cask/lib:/usr/local/Homebrew/Library/Homebrew -- /usr/local/Homebrew/Library/Homebrew/build.rb /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/cromwell.rb --verbose
==> Downloading https://github.com/broadinstitute/cromwell/releases/download/33.1/cromwell-33.1.jar
Already downloaded: /Users/brew/Library/Caches/Homebrew/cromwell-33.1.jar
==> Verifying cromwell-33.1.jar checksum
unzip -qq /Users/brew/Library/Caches/Homebrew/cromwell-33.1.jar -d /private/tmp/d20180720-32128-18ejwwt
warning [/Users/brew/Library/Caches/Homebrew/cromwell-33.1.jar]:  76 extra bytes at beginning or within zipfile
  (attempting to process anyway)
error [/Users/brew/Library/Caches/Homebrew/cromwell-33.1.jar]:  reported length of central directory is
  -76 bytes too long (Atari STZip zipfile?  J.H.Holm ZIPSPLIT 1.1
  zipfile?).  Compensating...
error:  expected central file header signature not found (file #133751).
  (please check that you have transferred or created the zipfile in the
  appropriate BINARY mode and that you have compiled UnZip properly)
Error: Failure while executing: unzip -qq /Users/brew/Library/Caches/Homebrew/cromwell-33.1.jar -d /private/tmp/d20180720-32128-18ejwwt

ghost assigned reitermarkus Jul 1, 2018

ghost added the in progress Maintainers are working on this label Jul 1, 2018

reitermarkus mentioned this pull request Jul 1, 2018

Tracking Issue: Merging download strategies. #4401

Closed

5 tasks

reitermarkus force-pushed the separate-staging branch from 9fe31e0 to 272c206 Compare July 1, 2018 21:45

reitermarkus requested review from MikeMcQuaid and JCount July 1, 2018 21:54

reitermarkus force-pushed the separate-staging branch from 272c206 to 031a3f7 Compare July 1, 2018 21:58

MikeMcQuaid requested changes Jul 2, 2018

View reviewed changes

reitermarkus force-pushed the separate-staging branch from 031a3f7 to 9484b53 Compare July 3, 2018 23:51

DomT4 mentioned this pull request Jul 4, 2018

env: move HOME away from buildpath/testpath #4355

Closed

5 tasks

reitermarkus mentioned this pull request Jul 5, 2018

Refactor staging step for download strategies. #4424

Merged

6 tasks

reitermarkus force-pushed the separate-staging branch 9 times, most recently from 27d629b to 2395008 Compare July 6, 2018 16:10

reitermarkus requested review from MikeMcQuaid and JCount and removed request for JCount July 6, 2018 16:22

MikeMcQuaid reviewed Jul 6, 2018

View reviewed changes

reitermarkus force-pushed the separate-staging branch 4 times, most recently from 8a0b5be to 8f55a8c Compare July 6, 2018 19:41

MikeMcQuaid reviewed Jul 8, 2018

View reviewed changes

reitermarkus force-pushed the separate-staging branch 4 times, most recently from ea208f4 to b0a58cb Compare July 10, 2018 23:57

reitermarkus force-pushed the separate-staging branch from b0a58cb to 7d9250d Compare July 11, 2018 16:43

MikeMcQuaid reviewed Jul 12, 2018

View reviewed changes

reitermarkus added 8 commits July 12, 2018 10:39

Separate staging from download.

5b3bbb7

Allow unused keyword arguments.

bf85611

Fix basename_without_params.

8ea7ad2

Refactor unpack strategy specs.

633c590

Fix XzUnpackStrategy.

ec7e22e

Reuse UncompressedUnpackStrategy#extract_to_dir.

5dfa4de

Add @ref_type and @ref needef for fossil.

cba55c8

xar is only pre-installed on macOS.

9b4e323

reitermarkus force-pushed the separate-staging branch from 7d9250d to 9b4e323 Compare July 12, 2018 08:39

JCount approved these changes Jul 12, 2018

View reviewed changes

MikeMcQuaid approved these changes Jul 12, 2018

View reviewed changes

reitermarkus merged commit aa3a369 into Homebrew:master Jul 12, 2018

ghost removed the in progress Maintainers are working on this label Jul 12, 2018

reitermarkus deleted the separate-staging branch July 12, 2018 19:28

reitermarkus mentioned this pull request Jul 12, 2018

Remove unnecessary :nounzips. Homebrew/homebrew-core#30011

Merged

4 tasks

ilovezfs mentioned this pull request Jul 18, 2018

kubernetes-cli 1.11.1 Homebrew/homebrew-core#30208

Closed

lock bot added the outdated PR was locked due to age label Aug 19, 2018

lock bot locked as resolved and limited conversation to collaborators Aug 19, 2018

Separate staging from download. #4402

Separate staging from download. #4402

Conversation

reitermarkus commented Jul 1, 2018 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus commented Jul 3, 2018

MikeMcQuaid commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus commented Jul 11, 2018

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus commented Jul 12, 2018

JCount left a comment

Choose a reason for hiding this comment

ilovezfs commented Jul 18, 2018

ilovezfs commented Jul 20, 2018

reitermarkus commented Jul 1, 2018 •

edited

Loading

reitermarkus Jul 6, 2018 •

edited

Loading