Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Implement formula revisions #19920

Closed
wants to merge 5 commits into from
Closed

Implement formula revisions #19920

wants to merge 5 commits into from

Conversation

jacknagel
Copy link
Contributor

This is an attempt to address #12190. It is very much a WIP.

Incrementing a formula's revision number prompts an upgrade. This should be done when something happens that necessitates recompilation (or equivalent). The revision is encoded in the keg name (except when revision == 0, for compatibility).

The most obvious example of where this is useful is when the library version of a dependency changes. In the last week this happened with icu4c, which broke harfbuzz and pango by association, and resulted in a lot of tickets. If we track revisions, we can just increment the revision number of harfbuzz and avoid this.

It will also be useful for propagating any formula maintenance that is significant enough to warrant a rebuild.

What's implemented is very basic and probably the implementation will change, I'm not in love with the Keg::Version class and I'd rather keep knowledge of revisions away from Keg. I'm toying with the idea of Formula#version returning an object that wraps Formula#active_spec.version and adds the revision tag. Edit: did this.

The details of how this will interact with devel are unclear, so I'm just punting on that for the time being. I'm mainly concerned with how it interacts with bottles, which already have their own revision system. I think it should work as-is, as long as we can ensure these two requirements:

(a) when the formula revision is incremented, a new bottle is made with the appropriate changes
(b) when a bottle is updated, the formula revision is incremented

IOW, (a) we must avoid incrementing the formula revision without rebottling, because we don't want to prompt a from-source upgrade; and (b) we want bottle revisions to be able to prompt upgrades.

This will necessitate some adjustments in the bottle tooling, as the formula revision is encoded in the keg name.

Anyway, I mostly just want a place for discussion, as I said the code is still a WIP.

@MikeMcQuaid
Copy link
Member

I'll give this a proper look/discussion soon. Moving house so busy with that at the moment.

@samueljohn
Copy link
Contributor

I looked through your diff's and I did not found a flaw. I really like the idea that a new version of a software allows us to remove the revision statement until we need to add revision 1 for some reason.

I have not looked, nor tested how this interacts with brew outdated, upgrade, or if this applies to how we name cached stuff or whatever.

@jacknagel
Copy link
Contributor Author

The change should be invisible to most code, as long as it just accesses Formula#version. So e.g. outdated will just work because it only does version comparisons and this is backwards compatible. upgrade gets its information from outdated, and in fact this is what makes the "incrementing the revision prompts an upgrade" just work.

@jacknagel
Copy link
Contributor Author

Responding to myself:

(b) when a bottle is updated, the formula revision is incremented

This won't work, because we don't want a bottle-only change to prompt an upgrade for people who have built from source; need to think about the interaction between revision and bottle.revision a bit more.

@MikeMcQuaid
Copy link
Member

Bottle revisions could be a point revision on normal revisions so $VERSION-$REVISION.$BOTTLE_REVISION or something.

@MikeMcQuaid
Copy link
Member

Yeh, after reading the code and having a think I think that's the best approach; the real version is like a major version, the revision is a minor revision and bottles are like patch revisions. If you've built from source then it doesn't try to upgrade patch/bottle revisions and if you've installed from a bottle then, as we have to rebuild bottles for revision bumps, it'll need rebottled and upgrade both bottles and source builds.

The one caveat I would say with this stuff (just to make sure we're agreed about current practise) is that not every change to a formula should necessarily require a new revision or bottle unless it's actually fixing a user issue.

@jacknagel
Copy link
Contributor Author

Yeah, it should only be used for compilation issues.

@jacknagel
Copy link
Contributor Author

Cool, I will work on adding the bottle revision as a patchlevel-like thing then.

@samueljohn
Copy link
Contributor

I could make good use of this for the python formula to put the recent fix in place. Good work!

@jacknagel
Copy link
Contributor Author

This is in usable shape now as far as I can tell.

@MikeMcQuaid This needs to be tested with brew bottle. It's set up so that during install --build-bottle it will increment the bottle revision and adjust the install prefix to match so it should be working, but it needs some testing.

@MikeMcQuaid
Copy link
Member

Will do.

@ghost
Copy link

ghost commented Jun 19, 2013

nettle/gnutls update will wait for this.

@MikeMcQuaid
Copy link
Member

Sorry about the delay in getting to this. brew bottle has a few failures; it'll need to use pkg_version instead of version and the bottle_filename and/or bottle_new_revision functions will need updated. Want me to get on them or are you happy to do so?

Stuff that should work:

brew install --build-bottle $FORMULA
brew bottle $FORMULA
brew install ./$BOTTLE_PATH

@jacknagel
Copy link
Contributor Author

Making this backwards compatible with existing bottles is somewhat difficult; we'll either need to add a flag to disable the new behavior on a per-formula basis, or re-bottle stuff before merging.

@MikeMcQuaid
Copy link
Member

Either sounds ok; the bots should make rebottling everything a lot easier. What will be the problem with existing bottles, out of interest? Bad tab/filename?

@jacknagel
Copy link
Contributor Author

The filenames will change, but that could be worked around; the main issue is the install prefixes are slightly different than what is currently embedded in the bottles.

@MikeMcQuaid
Copy link
Member

What would the new ones be?

@jacknagel
Copy link
Contributor Author

They would have the bottle revision baked into the prefix

@MikeMcQuaid
Copy link
Member

Even when it's zero? Can you give some examples?

@jacknagel
Copy link
Contributor Author

Ok, I worked on this a bit more. The revision and/or bottle revision are now omitted when it is zero, so existing bottles should be prefix-compatible. I haven't started updating the bottle machinery yet.

@MikeMcQuaid
Copy link
Member

Sounds good.

@MikeMcQuaid
Copy link
Member

Shout if/when you want some more review on this on the bottle side.

@camillol
Copy link
Contributor

camillol commented Aug 7, 2013

I think we should try to automate this first. When we install a formula, we already go through the dynamic linking information to fix the install names; it wouldn't be much of an additional burden to go through the libraries that are linked to, and write a record that the formula depends on those files. Then, when the dependency is upgraded/unlinked/uninstalled, we can quickly check which files went away and which formulas depend on them.

This is going to be useful even if we do decide to have manual revisions as well, because it would give us information on which we can base the decision of whether to bump the revision number or not.

@jacknagel
Copy link
Contributor Author

This is the standard approach used by most other package managers. Changes that require dependent upgrades happen at predictable boundaries.

Trust me, I considered all options here, and it's simply not worth the hassle.

@MikeMcQuaid
Copy link
Member

I'm not opposed to that if it solves problems. I guess it's not a regression of bottle functionality. I would say that I'm happy for us to just rebottle everything if that makes your life easier.

@jacknagel
Copy link
Contributor Author

Yeah, I mean, it's not really rebottling during the transition that's the issue, this problem happens independently of that.

And thinking about it more, it's probably the right way to do it anyway. For example, rebottling to add a relocatable bottle requires incrementing the bottle revision, but that shouldn't force an upgrade for users who already have it installed.

@mistydemeo
Copy link
Member

What's the status of this?

@jacknagel
Copy link
Contributor Author

One problem that remains is how to handle revisions for things that depend on symbol-style libpng, freetype, and fontconfig (i.e., dependencies that can be satisfied by X11 on OS X <= 10.6).

For example, if we update libpng to 1.6 and then increment the revision on cairo, all users will be prompted to upgrade cairo. That includes users on 10.6, who will not be prompted to upgrade libpng, because for these users, cairo’s dependency on libpng is satisfied by X11.

I see the following possible solutions:

(a) Assume that we are quickly approaching the day when the Apple-shipped versions of libpng, freetype, and fontconfig are mostly useless, and get rid of the symbol-style deps. Then all users have the same set of dependencies and the above problem goes away.

(b) Assume that (# of users on 10.7+) >> (# of users on <= 10.6), and accept that sometimes upgrades will be prompted when not actually necessary.

(c) Track extra metadata that can be used to skip revision-prompted upgrades for specific formulae. This means a bunch of added complexity and a special case to compensate for how we already special-case three formulae.

My preference is (a) or (b). I think (c) would be a lot of work for questionable gains.

@jacknagel
Copy link
Contributor Author

Other than that, I don't think there is anything left in terms of implementation other than making sure everything plays nicely with the bottle tooling.

@MikeMcQuaid
Copy link
Member

Assume that we are quickly approaching the day when the Apple-shipped versions of libpng, freetype, and fontconfig are mostly useless, and get rid of the symbol-style deps.

This sounds sensible to me. Really, unless you have X11 installed this is already the case. Considering b) would bite 10.6 most anyway (likely the slowest machines) this makes sense.

I have an idea of longer-term once we have more stuff being bottled (we're pretty close, now) of having the differentiation between a "library" and "application" formula and the latter (and maybe even the prior) installs all its dependences within its prefix (which pretty much makes this problem go away).

@mistydemeo
Copy link
Member

(a) Assume that we are quickly approaching the day when the Apple-shipped versions of libpng, freetype, and fontconfig are mostly useless, and get rid of the symbol-style deps. Then all users have the same set of dependencies and the above problem goes away.

This makes the most sense for me. I already have these as hard dependencies on 10.4, and have been considering doing so on 10.5; it's really a minority of older platforms where this is useful already.

@jacknagel
Copy link
Contributor Author

This makes the most sense for me. I already have these as hard dependencies on 10.4, and have been considering doing so on 10.5; it's really a minority of older platforms where this is useful already.

Good, I hoped this was the case.

@mistydemeo
Copy link
Member

That said, Tiger has a couple of symbol deps that are the opposite case (they don't activate on newer platforms), e.g. :ld64 and :expat, though of the two ld64 will never get a version bump anyway. Not a blocker either way.

@ghost
Copy link

ghost commented Feb 9, 2014

Oh gosh I'm so glad this has been merged finally! 😄 Thank you @jacknagel.

@MikeMcQuaid
Copy link
Member

@humdedum it hasn't?

@chdiza
Copy link
Contributor

chdiza commented Feb 9, 2014

Underneath the last batch of commits is a green rectangle that says "Merged build finished", which makes it seem as if the commits have been merged even though they haven't. I nearly fell for that too.

@ghost
Copy link

ghost commented Feb 12, 2014

Oops, sorry then!

@jacknagel jacknagel self-assigned this Mar 5, 2014
It is useful to be able to prompt upgrades in response to events other
than a version update; for example, when a dependency is updated and its
library version changes, dependents need to be rebuilt to link against
the new library.

Currently we cannot do this automatically, which means a flood of
tickets whenever the library version of a popular library changes.

To address this, we need to track an additional piece of metadata, the
"revision" of the formula, which can be incremented when appropriate to
prompt an upgrade. It can then be reset to zero when the next version
change occurs.
In order to allow kegs built with the same version but differing formula
revisions to coexist, we must encode the revision as part of the keg's
name. This is necessary to actually perform an upgrade, as we cannot
upgrade a keg in-place, and temporarily moving it pending the result of
the upgrade is error-prone and potentially slow.

To accomplish this, we introduce a new Formula#pkg_version method that
concatenates the active_spec version with the formula revision. An
exception is made for a formula that has no revision: the tag is
omitted. This preserves compatibility with existing installations.
@jacknagel
Copy link
Contributor Author

I'm currently doing some libpng 1.6 compatibility testing, and then over the next day or two I am going to roll this out.

/cc @Homebrew/owners

when :cairo, :pixman
# We no longer use X11 psuedo-deps for cairo or pixman,
# so just return a standard formula dependency.
when :cairo, :fontconfig, :freetype, :libpng, :pixman
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these trigger audit warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually, I want to go through the things in core first though.

@jacknagel
Copy link
Contributor Author

This is merged and I am rolling out an updated gnutls/nettle + dependents as a trial balloon.

@MikeMcQuaid
Copy link
Member

Merged? Awesome, congrats 👍

@Homebrew Homebrew 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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants