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

Add new extract dev command to retrieve old versions of formulae #4563

Merged
merged 25 commits into from Aug 26, 2018

Conversation

Projects
None yet
4 participants
@alebcay
Copy link
Member

alebcay commented Jul 29, 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?

This is my first time contributing to Homebrew/brew and in fact my first time ever writing Ruby outside of the Homebrew and Homebrew-Cask DSLs. So please bear with me - there's probably parts of this PR that are unidiomatic if not downright ugly. I may have re-invented the wheel a few times too.

This PR introduces the start of a brew extract command, intended to address #4055. It extracts the formula file for a specified formula that matches a given version and makes a copy of the file into the Formula subdirectory of a specified tap.

The syntax is along the lines of brew extract <formula> <version> <tap> and the new file is placed at <tap>/Formula/<formula>@<version>.rb. If the file already exists at that path, it will not overwrite the file unless --force is specified.

In it's current state, something is wrong with how I'm getting the "next-most recent" commit/revision to be considered. After a few iterations of searching via an until loop, the revisions are all empty, although if I write out a single iteration from the last good revision that it gave me, it'll still return me the proper next commit hash. It's almost as if the Git-querying part of the code stops responding after a certain amount of iterations in the loop but works perfectly fine outside of it. It's possible that I'm just overlooking some trivial Ruby-ism right now that's causing this, of course.


As a demonstration, the following command will work properly:

• brew extract --debug libogg 1.3.2 homebrew/core
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.3 from revision HEAD against desired 1.3.2
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.3 from revision ddb386b against desired 1.3.2
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.3 from revision f64a373 against desired 1.3.2
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision 16ebe5f against desired 1.3.2
==> Writing formula for libogg from 16ebe5f to /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg@1.3.2.rb

but the following will not:

• brew extract --debug libogg 1.3.1 homebrew/core
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.3 from revision HEAD against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.3 from revision ddb386b against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.3 from revision f64a373 against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision 16ebe5f against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision b100d2d against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision 23d09e4 against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision 8b378fd against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision 5ac836a against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision 893cd40 against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision 1fda5d2 against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.2 from revision f4bd9ce against desired 1.3.1
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> formulae require at least a URL in libogg at revision 61b2307
/usr/local/Homebrew/Library/Homebrew/formula.rb:244:in `determine_active_spec'
/usr/local/Homebrew/Library/Homebrew/formula.rb:197:in `initialize'
/usr/local/Homebrew/Library/Homebrew/formulary.rb:82:in `new'
/usr/local/Homebrew/Library/Homebrew/formulary.rb:82:in `get_formula'
/usr/local/Homebrew/Library/Homebrew/formulary.rb:327:in `from_contents'
/usr/local/Homebrew/Library/Homebrew/formula_versions.rb:40:in `block in formula_at_revision'
/usr/local/Homebrew/Library/Homebrew/utils.rb:425:in `nostdout'
/usr/local/Homebrew/Library/Homebrew/formula_versions.rb:40:in `formula_at_revision'
/usr/local/Homebrew/Library/Homebrew/cmd/extract.rb:37:in `extract'
/usr/local/Homebrew/Library/Homebrew/brew.rb:87:in `<main>'
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaContentsLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libogg.rb
==> Trying 1.3.3 from revision  against desired 1.3.1
Error: Could not find libogg 1.3.1.
/usr/local/Homebrew/Library/Homebrew/utils.rb:60:in `exit'
/usr/local/Homebrew/Library/Homebrew/utils.rb:60:in `odie'
/usr/local/Homebrew/Library/Homebrew/cmd/extract.rb:39:in `extract'
/usr/local/Homebrew/Library/Homebrew/brew.rb:87:in `<main>'

Other functionality that I've considered but am not sure whether or not would be a good idea to implement:

  • Allowing user to pass a git ref instead of a version (e.g. brew extract foo 1d9f4c3 homebrew/custom)
  • Searching through legacy homebrew repo(s) or user-specified taps as well (instead of just Homebrew/homebrew-core)
  • --stdout flag or something like that to just dump the formula on the terminal instead of to a file

Fixes #4055

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Fantastic work here @alebcay; despite all the nit picking there's no major problems here. Well done!

#: not installed yet, attempts to install/clone the tap before continuing.
#:
#: If the file at <tap>/Formula/<formula>@<version>.rb already exists,
#: it will not be overwritten unless `--force` is specified.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

Please document the --force option.

module Homebrew
module_function

def extract

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

Please use Homebrew::CLI::Parser.parse do (like audit.rb)

module_function

def extract
formula = Formulary.factory(ARGV.named[0])

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

ARGV.formulae.first and needs handling for if a no arguments are provided.

This comment has been minimized.

@alebcay

alebcay Jul 29, 2018

Member

Replacing this line with formula = ARGV.formulae.first gives the result Error: No available formula with the name "<version>", since it seems to be trying to parse all the named arguments as formulae.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

No worries, my mistake; didn't think about how you're using the other arguments. ARGV.name.first is sufficient (although you should check the value isn't nil first).


def extract
formula = Formulary.factory(ARGV.named[0])
version = ARGV.named[1]

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

Needs handling if no version is provided.

def extract
formula = Formulary.factory(ARGV.named[0])
version = ARGV.named[1]
dest = Tap.fetch(ARGV.named[2])

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

Needs handling if no dest is provided.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

destination_tap would be a nicer name.

To overwrite it and continue anyways, run `brew extract #{formula} #{version} #{dest.name} --force`.
EOS
end
ohai "Clobbering existing formula at #{path}" if ARGV.debug?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

Overwriting existing formula at


rev = "HEAD"
version_resolver = FormulaVersions.new(formula)
rev = Git.last_revision_commit_of_file(formula.path.parent.parent, formula.path, before_commit: "#{rev}~1") until version_resolver.formula_at_revision(rev) { |f| version_matches?(f, version, rev) || rev.empty? }

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

Use until do block form so this line is shorter

version_resolver = FormulaVersions.new(formula)
rev = Git.last_revision_commit_of_file(formula.path.parent.parent, formula.path, before_commit: "#{rev}~1") until version_resolver.formula_at_revision(rev) { |f| version_matches?(f, version, rev) || rev.empty? }

odie "Could not find #{formula} #{version}." if rev.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

End with !

ohai "Writing formula for #{formula} from #{rev} to #{path}"

# The class name has to be renamed to match the new filename, e.g. Foo version 1.2.3 becomes FooAT123 and resides in Foo@1.2.3.rb.
path.write result.gsub("class #{formula.name.capitalize} < Formula", "class #{formula.name.capitalize}AT#{version.gsub(/[^0-9a-z ]/i, "")} < Formula")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 29, 2018

Member

use result.gsub! and separate each gsub and write onto a separate line.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 29, 2018

Run brew man and commit the results. Would also be good to have an integration test for this (like others in Library/Homebrew/test/cmd/)

@commitay

This comment has been minimized.

Copy link
Contributor

commitay commented Jul 29, 2018

Should this be a dev command like create, edit and tap-new?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 29, 2018

Should this be a dev command like create, edit and tap-new?

@commitay Yep 👍

@alebcay alebcay changed the title Add new extract command to retrieve old versions of formulae Add new extract dev command to retrieve old versions of formulae Jul 29, 2018

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Jul 30, 2018

Reworked the code based on the nitpicks above. A UsageError is now raised if the wrong number of (named) arguments are provided (expect three when writing to tap, two when writing to stdout). A regular odie is thrown if the user tries to specify a tap and also use --stdout at the same time. This guards and simplifies me from having to ... unless <var>.nil? the three arguments separately.

Also generated and committed the manpages for the command. Tests still need to be written after we actually get the command working. The issue I originally stated regarding the strange behavior of last_revision_commit_of_file(repo, file, before_commit: nil) just not returning anything after a certain number of iterations is still happening.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Liking the changes, thanks again!


def extract
Homebrew::CLI::Parser.parse do
switch "--stdout", description: "Output to stdout on terminal instead of file"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 30, 2018

Member

I reckon just remove this argument for now to make things a little more simple until people ask for it explicitly.

end

odie "Cannot use a tap and --stdout at the same time!" if ARGV.named.length == 3 && args.stdout?
raise UsageError unless (ARGV.named.length == 3 && !args.stdout?) || (ARGV.named.length == 2 && args.stdout?)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 30, 2018

Member

An odie explaining what arguments are missing for each argument would be preferable.

This comment has been minimized.

@alebcay

alebcay Jul 30, 2018

Member

How would be a good way of going about figuring out which one is missing? i.e. how would I tell that brew extract libogg 1.3.2 is missing a tap compared to brew extract 1.3.2 homebrew/custom is missing a formula? Would this be easier if we explicitly specified each arg with a switch (like brew extract --formula libogg --version 1.3.2 --tap homebrew/custom)?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 30, 2018

Member

Yeh, you could perhaps make the tap the second argument rather than the third or something like --tap= and maybe even --version=.

destination_tap.install unless destination_tap.installed? || args.stdout?

unless args.stdout?
path = Pathname.new("#{destination_tap.path}/Formula/#{formula}@#{version}.rb")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 30, 2018

Member

I think you should make sure this destination tap is never Homebrew/homebrew-core.

unless ARGV.force?
odie <<~EOS
Destination formula already exists: #{path}
To overwrite it and continue anyways, run `brew extract #{formula} #{version} #{destination_tap.name} --force`.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 30, 2018

Member

Make sure this line isn't over 80 characters. I'd suggest doing run: and an indented newline

raise UsageError unless (ARGV.named.length == 3 && !args.stdout?) || (ARGV.named.length == 2 && args.stdout?)

formula = Formulary.factory(ARGV.named.first)
version = ARGV.named[1]

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 30, 2018

Member

Perhaps a missing version argument could extract the latest version of the formula (whether it currently exists or is deleted)

result = version_resolver.file_contents_at_revision(rev)

# The class name has to be renamed to match the new filename, e.g. Foo version 1.2.3 becomes FooAT123 and resides in Foo@1.2.3.rb.
result.gsub!("class #{formula.name.capitalize} < Formula", "class #{formula.name.capitalize}AT#{version.gsub(/[^0-9a-z ]/i, "")} < Formula")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 30, 2018

Member

Make each string here on a new line. Consider making formula.name.capitalize a variable and use Formulary.class_s(name) instead which will correctly generate the class from the name.

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Jul 30, 2018

stdout option is now removed (a tap must be specified via --tap=). The version is now optional (can be specified via --version=) and the most recent is taken if no version is specified. If the specified tap happens to be homebrew/core, it also bails. Message for re-running with --force now spans another line and should be within 80 chars. Lastly, cleaned up the class name generation part. We now perform just one gsub! in order to actually replace the first line of the file.

Still two major issues with this code as it stands:

  • Does not work at all with formulae that have been deleted. I think we'll have to stop relying on FormulaVersions and rely solely on Git.last_revision_commit_of_file(repo, file, before_commit: nil) and Git.last_revision_of_file(repo, file, before_commit: nil) to get those. It would be great if there would be some easy way to extract and compare the version from the file contents. I tried this and the only way I could think of was to put the contents of the last revision into a temp file and have Formulary read it from a path and see if it's the right version before deciding whether or not to dig further in the history. Hopefully there's a less kludgy way to go about this that I just don't know about.
  • Still have issues with Git.last_revision_commit_of_file(repo, file, before_commit: nil) deciding to stop working at times inside of loops.
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 31, 2018

Thanks @alebcay! I'll make a note to have a look at the Git stuff when I get a chance.

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Jul 31, 2018

I may have made some headway on that Git issue:

[34] brew(main)> resolver.formula_at_revision("1fda5d2") { |f| f.version }
=> #<Version::FromURL:0x00007f9100a747f0 @version="1.3.2">
[35] brew(main)> resolver.formula_at_revision("9274aca") { |f| f.version }
=> nil
[4] brew(main)> resolver.formula_at_revision("1fda5d2") { |f| f.name }
=> "libogg"
[5] brew(main)> resolver.formula_at_revision("9274aca") { |f| f.name }
=> nil

(9274aca is the next most recent commit after 1fda5d2 to have modified the formula in question - in this case, libogg)

At some point in the revision history, FormulaVersions.formula_at_revision(rev) is unable to get the formula anymore.


As another example, automake only has one "good" revision (1e1a871) before we have the same issue (61dae90):

[10] brew(main)> resolver.formula_at_revision("1e1a871") { |f| f.version }
=> #<Version::FromURL:0x00007fbdbb51dff8 @version="1.16.1">
[11] brew(main)> resolver.formula_at_revision("61dae90") { |f| f.version }
=> nil
[12] brew(main)> resolver.formula_at_revision("1e1a871") { |f| f.name }
=> "automake"
[13] brew(main)> resolver.formula_at_revision("61dae90") { |f| f.name }
=> nil
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 1, 2018

@alebcay It may be the older versions are invalid formulae i.e. use methods that have been since removed?

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 1, 2018

That's what I thought at first too, but the difference between working and non-working revisions of formulae is minute - the sha256 of the bottle being updated is somehow able to break things (in libogg's example).

Is there some kind of logic we could use to extract the URL from the formula file without constructing a proper Formula object, or better yet, easily extract the version from the URL from said formula? The first rather kludgy idea was to look for lines beginning with url and get the URL that way, but it's not very versatile since formulae can have resource blocks with URLs too.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 1, 2018

Is there some kind of logic we could use to extract the URL from the formula file without constructing a proper Formula object, or better yet, easily extract the version from the URL from said formula?

You could perhaps just handle method missing errors and make them no-ops; that would allow you to handle a case like this because none of them will affect the URL or version.

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 2, 2018

Another progress update - I've gotten the problems out of the way now, mostly by ignoring missing methods (sha1, md5) in Module and BottleSpecification. I may have gone a bit overboard with the monkeypatching/prepending - it's my first go with such a powerful tool and perhaps I'm using too blunt a tool for a fine operation. Right now, this code can go as far back as this commit before running into issues. Any ideas on how to reconcile/handle those older formulae, or are they just so old that I shouldn't bother?


This latest revision should also properly handle versions that don't exist (i.e. going "off the end" of the history, assuming those oldest commits can be parsed for a version) as well as those that have been deleted. You can try brew extract ecj --tap=<some/tap> to get the last version that existed (4.9) or brew extract ecj --tap=<some/tap> --version=4.5 to get an older version. An example of not being able to find a version is brew extract paq8px --tap=<some/tap> --version=68 (the oldest and only version available was 69 prior to deletion).


Still todo: fix style, write integration tests/spec.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Looking better, nice work!

# If some other number of args are given, provide generic usage information
raise UsageError if ARGV.named.length != 1

odie "The tap to which the formula is extracted must be specified!" if args.tap.nil?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

I wonder if it's worth making it a non-flagged argument (e.g. it's just the second argument)?

This comment has been minimized.

@alebcay

alebcay Aug 10, 2018

Member

I'll go in this direction. Just feels wrong making a flagged argument mandatory. In this case, we won't raise a separate FormulaUnspecifiedError, because there's no (easy) way to check if a missing argument is the missing formula or if it's the tap that needs to be specified. Instead, just a generic UsageError will be raised if we don't get exactly 2 named args.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 10, 2018

Member

Just feels wrong making a flagged argument mandatory

Agreed.

raise FormulaUnspecifiedError if ARGV.named.empty?

# If some other number of args are given, provide generic usage information
raise UsageError if ARGV.named.length != 1

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

What's the output look like for this?

This comment has been minimized.

@alebcay

alebcay Aug 6, 2018

Member
brew extract [--force] formula --tap=tap [--version=version]:
    Looks through repository history to find the version of formula and
    creates a copy in tap/Formula/formula@version.rb. If the tap is
    not installed yet, attempts to install/clone the tap before continuing.
    A tap must be passed through --tap in order for extract to work.

    If --force is passed, the file at the destination will be overwritten
    if it already exists. Otherwise, existing files will be preserved.

    If an argument is passed through --version, version of formula
    will be extracted and placed in the destination tap. Otherwise, the most
    recent version that can be found will be used.

Error: Invalid usage

This comment has been minimized.

@MikeMcQuaid
end

# If no formula args are given, ask specifically for a formula to be specified
raise FormulaUnspecifiedError if ARGV.named.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

What's the output look like for this?

This comment has been minimized.

@alebcay

alebcay Aug 6, 2018

Member
brew extract [--force] formula --tap=tap [--version=version]:
    Looks through repository history to find the version of formula and
    creates a copy in tap/Formula/formula@version.rb. If the tap is
    not installed yet, attempts to install/clone the tap before continuing.
    A tap must be passed through --tap in order for extract to work.

    If --force is passed, the file at the destination will be overwritten
    if it already exists. Otherwise, existing files will be preserved.

    If an argument is passed through --version, version of formula
    will be extracted and placed in the destination tap. Otherwise, the most
    recent version that can be found will be used.

Error: Invalid usage: This command requires a formula argument
name = formula.name
repo = formula.path.parent.parent
file = formula.path
rescue FormulaUnavailableError => e

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

Why bother with the non-rescue version of this code?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 10, 2018

Member

i.e. what advantage does the code above the rescue have over the code below?

core = Tap.fetch("homebrew/core")
name = ARGV.named.first.downcase
repo = core.path
file = core.path.join("Formula", "#{name}.rb")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

Prefer / over .join

This comment has been minimized.

@alebcay

alebcay Aug 10, 2018

Member

Would core.path.join("Formula/#{name}.rb") be all right? Or just remove it entirely, but create Pathname object around the whole thing (Pathname.new("#{core.path}/Formula/#{name}.rb"))? At some point, it'll need to be converted into a Pathname because Git.last_revision_of_file(repo, file, before_commit: rev) expects arguments as Pathname objects, not String.

This comment has been minimized.

@reitermarkus

reitermarkus Aug 10, 2018

Member

core.path/"Formula/#{name}.rb"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 10, 2018

Member

(which returns a Pathname)

destination_tap = Tap.fetch(args.tap)
destination_tap.install unless destination_tap.installed?

odie "Cannot extract formula to homebrew/core!" if destination_tap.name == "homebrew/core"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

Would be good to put this up with the other argument handling.

This comment has been minimized.

@reitermarkus

reitermarkus Aug 5, 2018

Member

destination_tap.core_tap?


odie "Cannot extract formula to homebrew/core!" if destination_tap.name == "homebrew/core"

if args.version.nil?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

if args.version and flip it?


# @private
def formula_at_revision(repo, name, file, rev)
return nil if rev.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

return if rev.empty? returns nil by default

file = formula.path
rescue FormulaUnavailableError => e
opoo "'#{ARGV.named.first}' does not currently exist in the core tap" if ARGV.debug?
core = Tap.fetch("homebrew/core")

This comment has been minimized.

@reitermarkus

reitermarkus Aug 5, 2018

Member

CoreTap.instance

destination_tap = Tap.fetch(args.tap)
destination_tap.install unless destination_tap.installed?

odie "Cannot extract formula to homebrew/core!" if destination_tap.name == "homebrew/core"

This comment has been minimized.

@reitermarkus

reitermarkus Aug 5, 2018

Member

destination_tap.core_tap?

end
end

class Module

This comment has been minimized.

@reitermarkus

reitermarkus Aug 5, 2018

Member

Dont' monkey-patch this.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 5, 2018

Member

@reitermarkus Do you have an alternate suggestion to handling old formulae?

This comment has been minimized.

@reitermarkus

reitermarkus Aug 5, 2018

Member

We can use refinements to limit the monkey-patch to a certain scope.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 6, 2018

Member

Seems like a good idea 👍

require "formulary"
require "tap"

class BottleSpecification

This comment has been minimized.

@reitermarkus

reitermarkus Aug 5, 2018

Member

Dont' monkey-patch this.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 23, 2018

Member

@reitermarkus Are you OK with the current implementation given the discussion?

This comment has been minimized.

@MikeMcQuaid
end
end

class DependencyCollector

This comment has been minimized.

@reitermarkus

reitermarkus Aug 5, 2018

Member

Dont' monkey-patch this.

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 10, 2018

Made a feeble attempt at using Ruby refinements instead of just monkeypatching. In this state they don't work (it complains about sha1 etc. not found on older formulae, which is what the monkeypatches fixed). I'm probably overlooking some nuance of refinements, but figured I'd update my progress here instead of just banging my head on the wall.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 10, 2018

Oh right! The refinements are scoped, so it won't work if a missing method on a BottleSpecification gets called from another file. 🤦‍♂️

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 11, 2018

Would it be all right just to do a monkey-patch like before then?

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 16, 2018

All right, another batch of changes! This version now works on formulae as old as the homebrew-core repo itself (i.e. all the way back to the initial commit in the repo). I'll also get around to starting on writing up spec/tests.


class BottleSpecification
def method_missing(m, *_args, &_block)
# no-op

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 16, 2018

Member

Can skip all the no-op comments.

require "tap"

class BottleSpecification
def method_missing(m, *_args, &_block)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 16, 2018

Member

Can just do method_missing(*) if you're not using any parameters

when :java then JavaRequirement.new(tags)
when :osxfuse then OsxfuseRequirement.new(tags)
when :tuntap then TuntapRequirement.new(tags)
when :ld64 then ld64_dep_if_needed(tags)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 16, 2018

Member

Make all these values a no-op instead.


module Compat
def parse_string_spec(spec, tags)
opoo "'depends_on ... => :run' is disabled. There is no replacement." if tags.include?(:run) && ARGV.debug?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 16, 2018

Member

Can just likely make this a super


core = CoreTap.instance
name = ARGV.named.first.downcase
repo = core.path

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 16, 2018

Member

repo = CoreTap.instance.path

odie "Cannot extract formula to homebrew/core!" if destination_tap.core_tap?
destination_tap.install unless destination_tap.installed?

core = CoreTap.instance

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 16, 2018

Member

Can remove this

core = CoreTap.instance
name = ARGV.named.first.downcase
repo = core.path
file = core.path/"Formula/#{name}.rb"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 16, 2018

Member

file = repo/"Formula/#{name}.rb" although it's worth noting not all repos have this structure (the Formula directory is optional)

This comment has been minimized.

@alebcay

alebcay Aug 16, 2018

Member

I'm aware that the Formula dir isn't required, although I figured that it would be fine since we're only reading from the core tap, which does have the directory. Should I consider generalizing this logic a bit so that reading from other taps (which might not have the Formula dir) is possible down the road?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 17, 2018

Member

@alebcay Yeh, it's fine, might want to note it as a comment though just so others looking at the code are aware 👍

result = Git.last_revision_of_file(repo, file)
end

# The class name has to be renamed to match the new filename, e.g. Foo version 1.2.3 becomes FooAT123 and resides in Foo@1.2.3.rb.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 16, 2018

Member

This line needs wrapped, it's a bit too long.

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 20, 2018

Addressed CR points again. Added a starter file for the spec, but it doesn't work yet. Still trying to figure it out, but if someone else wants to take a look I'd appreciate any help.

require "tap"

class BottleSpecification
def method_missing(*)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 21, 2018

Member

Could even do def method_missing(*); end if you wanted to be even lighter for these.

end

class DependencyCollector
def parse_symbol_spec(spec, tags)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 21, 2018

Member

Could also use (*) on these too.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 21, 2018

@alebcay Can you rebase this on master? Thanks! Not sure why the tests aren't running but that'll fix it.

@alebcay alebcay force-pushed the alebcay:extract branch from 244207f to 748064c Aug 21, 2018

alebcay added some commits Jul 29, 2018

Fix up extract command
- Move from cmd to dev-cmd
- Add --stdout flag
- Add sanity checks for args/flags
- Minor rewording on error messages
Additional fixups for extract command
- Rework command line options
- Make specifying a version optional
- Remove stdout option and require a tap to be specified
- Do not allow user to extract into homebrew/core
- Rework new class name generation to use existing Formulary tools
@@ -0,0 +1,235 @@
describe "brew extract", :integration_test do

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 25, 2018

Member

Integration tests are comparatively really slow. As a result I think it's sufficient to have a single test that verifies the happy path. If you wish to add more tests consider refactoring some logic out of cmd and into something that can be unit tested (but I think a single test is fine here for now).

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 25, 2018

Member

This was addressed 👍

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 25, 2018

All style issues should be fixed now. I heavily reduced/combined the integration tests. The corner cases would indeed probably do better in a unit test. I have considered refactoring parts of the logic in this feature out of dev-cmd (into somewhere else in the core tree), but figured that it really isn't much use to anyone/anything else at the moment.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 25, 2018

@alebcay The code looks good! Feedback from having run it and some suggested, very small final changes.

  • brew extract inkscape homebrew/bundle pauses for a long time before failing with Error: inkscape: undefined method sha1' for #Resource::PatchResource:0x000000010207cc88. That error might want to be addressed but I'd suggest at least making it print a message (with ohai`) indicating that it's e.g. searching the Git history?
  • brew extract wget homebrew/bundle --version=1.16.1 worked as expected but brew install wget@1.16.1 failed due to undefined method sha1' for #Class:0x00000001011dafe0. I wonder if it's worth stripping the bottle blocks or e.g. sha1` etc.? I think it's fine to say "good idea for a follow up PR (by someone else)".
  • brew extract wget homebrew/bundle --version=1.19.2 and brew install wget@1.19.2 worked as expected.
  • I think it's worth adding a reference to this command in https://github.com/Homebrew/brew/blob/master/docs/Versions.md. That's exactly the type of resource we point people to to justify why we won't provide every version; it'd be perfect to recommend it there.
@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 25, 2018

For unit-testing, the monkey-patches should be reversible. This could be done with something like this:

def with_monkey_patch
  BottleSpecification.class_eval do
    if method_defined?(:missing_method)
      alias_method :missing_method, :old_missing_method
    end
  end

  yield
ensure
  BottleSpecification.class_eval do
    if method_defined?(:old_missing_method)
      alias_method :old_missing_method, :missing_method
      undef :old_missing_method
    end
  end
end

alebcay added some commits Aug 25, 2018

extract: better success message
Fixes the empty rev string when copying file from HEAD; explicitly
set rev to HEAD in that case.
extract: add progress message when searching Git history
For the impatient ones out there.
@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 25, 2018

brew extract inkscape homebrew/bundle pauses for a long time before failing

Added a progress message. The failure was because we needed to monkey-patch method_missing(*) in PatchResource. I just did the monkey-patch on Resource instead to cover any other corner cases where resource checksums were sha1 instead of sha256. If you can think of anywhere else in the codebase where we've been using checksums and have upgraded from sha1/md5 to sha256, they also probably need a monkey-patch.

brew extract wget homebrew/bundle --version=1.16.1 worked as expected but brew install wget@1.16.1 failed

I suppose a question to ask is - how usable are we expecting formula that this churns out to be? We could certainly strip sha1/other missing blocks, but edits would have to be made to add a sha256 block too, right? I think it's beyond the scope of this command (and definitely outside of the scope of this PR) to have the extract command not only retrieve, but also format old Homebrew stanzas/replace them with modern alternatives.

At least, I was under the assumption that the formulae that are churned out here are "use at your own risk" or "needs some fixing up" by the end user. I'm already a bit concerned that we're packing a lot of logic into a single command, and if we were to add this other stuff, I think it may warrant refactoring some of the stuff out of dev-cmd.

To be clear, I don't mind stripping old stuff out of the resulting formulae, but I just don't feel that it's much of an improvement if we don't replace them with their modern equivalents - and that part is considerably more complicated.

I'll get around to putting a word in the docs about the command.

For unit-testing, the monkey-patches should be reversible.

Where would this thing go? In the extract_spec.rb, and I'd just call with_monkey_patch from somewhere? Please forgive me for being dense. 😊

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 25, 2018

Where would this thing go?

Loading the formula would happen inside with_monkey_patch, so the monkey-patch is only active when loading an old formula.

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 25, 2018

Loading the formula would happen inside with_monkey_patch, so the monkey-patch is only active when loading an old formula.

Gotcha. Ruby is such a nifty language!

extract: use localized monkey-patching
Instead of just blanketing over with monkey-patches like before,
set up monkey-patches as needed, and make sure to clean up after
we're done.
require "formulary"
require "tap"

def with_monkey_patch

This comment has been minimized.

@reitermarkus

reitermarkus Aug 25, 2018

Member

Also add the DependencyCollector monkey-patch here.

def parse_symbol_spec(*); end

module Compat
def parse_string_spec(spec, tags)

This comment has been minimized.

@reitermarkus

reitermarkus Aug 25, 2018

Member

What is this doing that def parse_symbol_spec(*); end doesn't?

This comment has been minimized.

@alebcay

alebcay Aug 25, 2018

Member

I was trying to monkey-patch over this compat definition of parse_symbol_spec; the super one works fine as far as I can tell.

Simply no-oping it here still results in an error:

Error: Calling 'depends_on ... => :run' is disabled! There is no replacement.

We need to override the prepend somehow.

This comment has been minimized.

@reitermarkus

reitermarkus Aug 25, 2018

Member

Ok, you can do DependencyCollector.class_eval and DependencyCollector::Compat.class_eval for this. And we don't need prepend Compat, since this is already done by the file in compat/.

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 26, 2018

Done, although tests are failing because the new monkey-patch doesn't work now with HOMEBREW_NO_COMPAT=1:

Error: uninitialized constant DependencyCollector::Compat

I assume it's because we're trying to modify DependencyCollector::Compat, from compat/, which isn't loaded when HOMEBREW_NO_COMPAT=1.

Should I just guard the monkey-patch to check the env variable, or just require the relevant file?

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 26, 2018

You could check for defined?(DependencyCollector::Compat).

@MikeMcQuaid MikeMcQuaid merged commit 9bc17c8 into Homebrew:master Aug 26, 2018

3 checks passed

codecov/patch 77.88% of diff hit (target 71.15%)
Details
codecov/project 71.2% (+0.05%) compared to 713532e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 26, 2018

Thanks for all the hard, great work here @alebcay!

To be clear, I don't mind stripping old stuff out of the resulting formulae, but I just don't feel that it's much of an improvement if we don't replace them with their modern equivalents - and that part is considerably more complicated.

Agreed this can be done in a future PR if someone needs it (and not necessarily by you).

I'll get around to putting a word in the docs about the command.

That'd be great, thanks!

@alebcay

This comment has been minimized.

Copy link
Member

alebcay commented Aug 26, 2018

Thanks for walking me through the process! Also thanks to @reitermarkus for help with all the Ruby-isms.

@alebcay alebcay referenced this pull request Aug 26, 2018

Merged

extract: update docs #4754

6 of 6 tasks complete

@alebcay alebcay deleted the alebcay:extract branch Aug 28, 2018

@lock lock bot added the outdated label Sep 27, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Sep 27, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.