Improving version handling #13308

Closed
jacknagel opened this Issue Jul 10, 2012 · 24 comments

3 participants

@jacknagel

To facilitate a better outdated comparison, I've started working on overhauling how versions are passed around:

https://github.com/jacknagel/homebrew/tree/specs

The base comparison should handle most common version strings, but I plan to extend it so that a custom comparator can be supplied.

Other things I would like to do:

  • Totally decouple version detection from Pathname. It's tough, because a lot of breaking down the string is done by the extname/stem pathname methods. I thought about converting all URLs into URI objects so that we could use URI#host and URI#path, but it's a pretty daunting task. May be worth it eventually, though.

  • Give the Version class some inspection methods that brew audit can use, e.g. Version#devel? could be used to warn that a user is sticking an unstable URL in the stable URL field. Version#from_url? (or Version#detected?) would allow me to get rid of the ugly SoftwareSpec#explicit_version? hack.

But the main goal is to make the outdated comparison work as most users expect that it should.

Any suggestions are welcome.

@jacknagel

Support for custom comparators will look something like this:

  • Any formula requiring a custom version comparator should declare an explicit version.
  • The version DSL will be extended to take a Hash, with the version as the key and the comparator as the value. The comparator should specify a class, but eventually it will support symbols. e.g.:
version 'R13B' => ErlangVersionScheme
version 'R13B' => :erlang
  • Custom comparators will take the form of classes that inherit from Version and reimplement <=>.
@jacknagel

The version detection still needs work. The result of Version.parse (mostly what used to be Pathname#version) is highly dependent on what order the regexps are used to match the string. I switched a few around while doing moving the code, and it resulted in a bunch of new redundant versions in formulae. Who knows how many it caused to be misdetected.

I'd like to fix that, but I think we may want to consider being a bit more strict in what we accept, and require more explicit versions.

@jacknagel

Also I imagine that we will eventually want to use Version objects internally for stuff like MacOS.xcode_version.

@jacknagel

Pushed again; brew outdated now works as it should.

@jacknagel

Also I implemented the custom comparator stuff.

@samueljohn

Your changes to brew outdated also fix the wrong "outdated" for newer versions from taps. For example suite-sparse from staticfloat/julia provides 4.0.0.

> brew outdated
suite-sparse (4.0.0 < 3.7.0)

That is fixed with @jacknagel specs branch. Good job.

I don't know if that is related or not, but "brew missing" thinks I need to install suite-sparse:

> brew missing
julia: suite-sparse

That is not the case for the current master.

@jacknagel

I'll look into it, thanks for the heads up.

@jacknagel

brew missing calls into the outdated code, so my changes probably broke it. I'll put it on the list.

@jacknagel

Pushed again,

https://github.com/jacknagel/homebrew/tree/specs

mostly just incremental improvements, haven't done anything major since the last time I pushed.

@jacknagel

Repushed: https://github.com/jacknagel/homebrew/compare/specs

I think it is pretty much ready for review.

@mxcl
Homebrew member

Great.

Ideally we'd have a way to check that the latest version of a formula's version passes >= the previous version's version.

In fact, it would be cool to do a test before merging this where we instantiate all previous formulas and check that the version for each is < than the next.

When I continue with the auto-audit feature I'll make it do the first suggestion.

Good work.

@jacknagel

Yeah. There is some work remaining here, as I'm sure the default comparison fails for some version strings. Those formulae will eventually need to have a comparator defined:

class FooVersion < Version
  compare do |other|
    # do the comparison...
  end
end

class Foo < Formula
  version 'blah' => FooVersion
end

but I haven't spent any time sniffing out things with really ugly version strings yet, and I'm going to hold off until this has had some time to cook in production.

@jacknagel

@samueljohn I think I fixed the brew missing problem, can you apply this branch and test it again?

@jacknagel

@mistydemeo @mikemcquaid I'd like one or two more ACKs here before I merge anything. Though it is pretty well tested.

@samueljohn

@jacknagel Testing your specs branch here I get a strange behavior when brew upgrade but not sure if related to your changes.

12:25:14 engage:/usr/local[jacks_specs✓] ——> brew outdated
x264 (r2197 < r2197.4)
12:25:17 engage:/usr/local[jacks_specs✓] ——> brew upgrade
Error: no block given
Please report this bug:
    https://github.com/mxcl/homebrew/wiki/bug-fixing-checklist
/usr/local/Library/Homebrew/cmd/outdated.rb:22:in `outdated_brews'
/usr/local/Library/Homebrew/cmd/outdated.rb:17:in `each'
/usr/local/Library/Homebrew/cmd/outdated.rb:17:in `outdated_brews'
/usr/local/Library/Homebrew/cmd/upgrade.rb:21:in `upgrade'
/usr/local/bin/brew:80:in `send'
/usr/local/bin/brew:80
@samueljohn

Also:

14:13:23 engage:.../local/Library/Formula[jacks_specs✓] ——> brew versions wget
Error: undefined method `ljust' for #<Version:0x10fabc7b8 @version="1.14", @detected_from_url=true>
Please report this bug:
    https://github.com/mxcl/homebrew/wiki/bug-fixing-checklist
/usr/local/Library/Homebrew/cmd/versions.rb:16:in `versions'
/usr/local/Library/Homebrew/cmd/versions.rb:32:in `versions'
/usr/local/Library/Homebrew/cmd/versions.rb:29:in `each'
/usr/local/Library/Homebrew/cmd/versions.rb:29:in `versions'
/usr/local/Library/Homebrew/cmd/versions.rb:14:in `versions'
/usr/local/Library/Homebrew/macos.rb:6:in `all?'
/usr/local/Library/Homebrew/cmd/versions.rb:10:in `each'
/usr/local/Library/Homebrew/cmd/versions.rb:10:in `all?'
/usr/local/Library/Homebrew/cmd/versions.rb:10:in `versions'
/usr/local/bin/brew:80:in `send'
/usr/local/bin/brew:80
@samueljohn

@jacknagel but besides that, I could not find any other problems. The issue with "--devel" installed stuff showing up in brew missing is either already fixed in the current master or was by accident due to the version bumping from "0.9.1" to "0.10.1". And the latter version numbers is definitely still a problem in the current master. But your specs branch fixes this for me.
This mistake pig (0.10.0 < 0.9.2) is no longer shown!

@jacknagel

Probably responsible for both errors. Thanks for giving it a test run.

@jacknagel

Reproduced and fixe both issues; repushed.

@samueljohn

Good, so far!
Could this be an issue from your new code? I don't seem to see this when in the master branch.

@jacknagel

Yep; the gcc formula in Homebrew-dupes is calling string methods on version without calling to_s first.

That is the one backwards-incompatible change that can't really be worked around. There isn't a String mixin afaik so hopefully it doesn't break too many things. At least they are simple fixes.

@jacknagel

This is now merged.

@jacknagel jacknagel closed this Aug 18, 2012
@samueljohn

Thank you for this!

@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.