Implement version_schemes

[vladshablinsky]

• 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 tests with your changes locally?

---

Issue: Homebrew/legacy-homebrew#41563

The idea is to add epoch to DSL and reimplement outdated_versions-related things to be able to change version schemes, thus epochs are quite similar to revisions, but we store them in tabs.

CC @xu-cheng

[MikeMcQuaid]

Can you split this into multiple statements/lines somehow? Quite a bit of logic being chained here; perhaps having 2-3 separate variables would help.

[vladshablinsky]

I can come up with this:

  if !version.head? 
   return [] if epoch == tab.epoch && pkg_version <= version
  end

[MikeMcQuaid]

How about:

outdated_version = pkg_version <= version
equal_epoch = epoch == tab.epoch
not_head_version = !version.head?
return [] if outdated_version && equal_epoch && not_head_version

[vladshablinsky]

outdated_version is actually not_outdated_version.

I don't really like not prefixes, but If you're OK with them I make the change. Also, equal_epoch can be done same_epoch.

[MikeMcQuaid]

latest_version, same_epoch, not_head perhaps (as I can't think of a way of decribing "not head" without "not").

[vladshablinsky]

👍 will make the change.

[MikeMcQuaid]

Great work on this, @vladshablinsky. Could you make a PR to homebrew-core with an example that we could use to test (and merge after this is done) and some commands to run to test this?

[MikeMcQuaid]

CC @DomT4 too who will be interested in this.

[DomT4]

Thanks for working on this @vladshablinsky ❤️. osh is a formula in homebrew/core that is known to need epochs to deal with a version change shift, so if you want to play with one that'd be a solid candidate possibly.

Will play with this later tonight locally a bit; currently working on GPG stuff, which is being "fun".

[MikeMcQuaid]

I tested this with Homebrew/homebrew-core#3900 and it worked great. I installed the 20160726 version, applied Homebrew/homebrew-core#3900 as a patch, removed the custom version stuff in there in favour of a epoch 1, verified that brew outdated and brew upgrade showed it as outdated and upgraded to 1.0.0, uninstalled the 20160726 version manually, reverted Homebrew/homebrew-core#3900, added epoch 2, verified that brew outdated and brew upgrade showed it as outdated and upgraded to 20160726.

As far as I'm concerned this is good to 🚢. The only wording I might want to adjust is epoch to something more obvious like version_scheme, version_scheme_revision, version_scheme_changed, version_scheme_version etc. The overhead of making this more readable is very low and makes it much more obvious what it's doing, conceptually.

[MikeMcQuaid]

Also: I want to get this merged sooner rather than later so if @DomT4 agrees on one of those names let's try and get this shipped this week.

[vladshablinsky]

Will change it to version_scheme for now, I like this option the most.
Also, I'd like to say that this way of handling epochs was suggested by @xu-cheng, so thanks to him as well.

[xu-cheng]

I think this should be:

latest_version = pkg_version <= version
latest_version_scheme = version_scheme <= tab.version_scheme
not_head = !version.head?
return [] if (latest_version || latest_version_scheme) && not_head

[MikeMcQuaid]

Let's pull it out to:

latest_version = pkg_version <= version
latest_version_scheme = version_scheme <= tab.version_scheme
latest_version_or_scheme = latest_version || latest_version_scheme
not_head = !version.head?
return [] if latest_version_or_scheme && not_head

(to simplify humans parsing the binary logic)

[xu-cheng]

How about:

 installed_kegs.each do |keg|
  version = keg.version
  tab = Tab.for_keg(keg)
  all_versions << version
  next if version.head?
  next if version_scheme > tab.version_scheme
  next if pkg_version > version
  return []
end

[MikeMcQuaid]

Even better. Could skip creating the tab until after version.head? when we know it's needed, too.

[xu-cheng]

Actually, I just realize I gave an incorrect suggestion in the first place. The correct way is:

installed_kegs.each do |keg|
  version = keg.version
  tab = Tab.for_keg(keg)
  all_versions << version
  next if version.head?
  next if version_scheme > tab.version_scheme
  next if version_scheme == tab.version_scheme && pkg_version > version
  return []
end

Noted that the correct way to compare two versions is:

(version_scheme <=> other.version_scheme).nonzero? || pkg_version <=> other.pkg_version

[MikeMcQuaid]

Would be good to make next if version_scheme == tab.version_scheme && pkg_version > version into two variables which are && to again make the boolean logic better.

[MikeMcQuaid]

And, again, moving Tab.for_keg(keg) after version.head? so it's not read unnecessariy.

[DomT4]

I like epoch because the concept should be immediately slightly more familiar to anyone who has encountered Debian's packaging before, but if the cost of getting this done is losing epoch it's not the worst thing in the world.

I can live with version_scheme. It feels a bit strange to write but I suspect that's down to my usage of epoch for the last year every time this discussion has come up.

I think it's worth adding audit code similar to our revision check to ensure that epochs or whatever it comes to be called are never removed from formulae, because obviously once it's there if a contributor removes it that'll cause upgrade problems and that may not be immediately obvious to every contributor, especially since we do remove revisions on version updates.

[MikeMcQuaid]

I like epoch because the concept should be immediately slightly more familiar to anyone who has encountered Debian's packaging before

FWIW I'd done Debian packaging before and never heard epoch used this way so I think this is catering for a very small group of people.

I think it's worth adding audit code similar to our revision check to ensure that epochs or whatever it comes to be called are never removed from formulae, because obviously once it's there if a contributor removes it that'll cause upgrade problems and that may not be immediately obvious to every contributor, especially since we do remove revisions on version updates.

Agreed (but in a follow-up PR).

[DomT4]

Agreed (but in a follow-up PR).

Doesn't need to be a blocker here but would like it chased up on before we start using this in homebrew/core. Documentation in the Cookbook would be a 👍 as well, although obviously there's broader changes to come there if we're going to be finally renaming revisions et al.

[MikeMcQuaid]

Tested this again locally and it worked great. Nice work @vladshablinsky 👏

[MikeMcQuaid]

Doesn't need to be a blocker here but would like it chased up on before we start using this in homebrew/core. Documentation in the Cookbook would be a 👍 as well, although obviously there's broader changes to come there if we're going to be finally renaming revisions et al.

Done in #743.

[dunn]

I saw this too late, but IMO version_scheme is a misleading name since it suggests a scheme, whereas what we've got is something more like an iteration or, well, an epoch.

[MikeMcQuaid]

@dunn It's a migration between versioning schemes e.g. 0 is one versioning scheme, 1 is another etc.

[dunn]

Oh I know, but to me it suggests the DSL is for specifying the scheme, rather than the versioning scheme ... version 😸

[ilovezfs]

Are updates to taps occurring before updates to the main brew code?

Josephs-MacBook-Pro:~ joe$ brew update
Updated Homebrew from db2e9b8 to 5c7c9de.
Error: undefined method `version_scheme' for #<Class:0x007fb2ab9d91e8>
/usr/local/Library/Taps/homebrew/homebrew-core/Formula/pyenv.rb:6:in `<class:Pyenv>'
/usr/local/Library/Taps/homebrew/homebrew-core/Formula/pyenv.rb:1:in `load_formula'
/usr/local/Library/Homebrew/formulary.rb:25:in `module_eval'
/usr/local/Library/Homebrew/formulary.rb:25:in `load_formula'
/usr/local/Library/Homebrew/formulary.rb:42:in `load_formula_from_path'
/usr/local/Library/Homebrew/formulary.rb:91:in `load_file'
/usr/local/Library/Homebrew/formulary.rb:82:in `klass'
/usr/local/Library/Homebrew/formulary.rb:78:in `get_formula'
/usr/local/Library/Homebrew/formulary.rb:215:in `factory'
/usr/local/Library/Homebrew/cmd/update-report.rb:212:in `block in report'
/usr/local/Library/Homebrew/cmd/update-report.rb:190:in `each_line'
/usr/local/Library/Homebrew/cmd/update-report.rb:190:in `report'
/usr/local/Library/Homebrew/cmd/update-report.rb:370:in `add'
/usr/local/Library/Homebrew/cmd/update-report.rb:62:in `block in update_report'
/usr/local/Library/Homebrew/tap.rb:478:in `block (2 levels) in each'
/usr/local/Library/Homebrew/tap.rb:477:in `each'
/usr/local/Library/Homebrew/tap.rb:477:in `block in each'
/usr/local/Library/Homebrew/tap.rb:476:in `each'
/usr/local/Library/Homebrew/tap.rb:476:in `each'
/usr/local/Library/Homebrew/cmd/update-report.rb:52:in `update_report'
/usr/local/Library/Homebrew/brew.rb:87:in `<main>'
Updated 5 taps (caskroom/cask, homebrew/bundle, homebrew/core, homebrew/php, homebrew/versions).
==> New Formulae
homebrew/php/php70-gearman        homebrew/php/php71-snmp           homebrew/php/php71-timecop      
homebrew/php/php71-oauth          homebrew/php/php71-stats          homebrew/php/php71-xxtea        
homebrew/php/php71-pcntl          homebrew/php/php71-swoole         homebrew/versions/go16          
homebrew/php/php71-pspell         homebrew/php/php71-tidy         
==> Updated Formulae
homebrew/php/phpmyadmin                            pyenv                                            
Josephs-MacBook-Pro:~ joe$ 

[MikeMcQuaid]

@ilovezfs They are not but did you have a non-master Homebrew/brew branch checked out?

[ilovezfs]

ah nevermind I think I know what happened.

[MikeMcQuaid]

Also that exception is only printed for developers.

[ilovezfs]

@MikeMcQuaid yeah, that particular tree is on non-master since it has my version of bump-formula-pr.

[MikeMcQuaid]

my version of bump-formula-pr.

PR requested 😉