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

Upgrade: implement linkage repair #4767

Merged
merged 1 commit into from Sep 13, 2018

Conversation

Projects
None yet
5 participants
@amyspark
Copy link
Member

amyspark commented Aug 27, 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?

After upgrade, verify that existing kegs are still linked properly. If not, reinstall them.

Fixes #4761.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

I'm amazed you managed to get to this so quickly, great work here!

@@ -104,6 +105,35 @@ def upgrade
onoe "#{f}: #{e}"
end
end

unless Homebrew.failed?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 28, 2018

Member

return if Homebrew.failed?

This comment has been minimized.

@amyspark

amyspark Aug 28, 2018

Member

We cannot return yet, as we have not printed the caveats of the upgraded casks yet.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 28, 2018

Member

@amyspark sorry, misread, you're right!

Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated
Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated

odebug "Assessing keg linkage status" if kegs.size > 1

kegs.each do |keg|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 28, 2018

Member

Could consider slimming this down to only those formulae with runtime_dependencies on this formula (which indicates linkage already and is much faster than reading linkage on all kegs). Thoughts?

This comment has been minimized.

@amyspark

amyspark Aug 28, 2018

Member
Formula.installed
  .collect(&:opt_or_installed_prefix_keg)
  .keep_if {...}

Two options for checking here:

  • |k| k.runtime_dependencies.contains?(f);
  • |k| k.runtime_dependencies.map(&:name).include?(f.full_alias_name).

I went for the first one, but let me know which is correct!

Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated
@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 28, 2018

In general @MikeMcQuaid -- how does one use FormulaInstaller for rebuilding from source? Should I duplicate the logic from cmd/reinstall and just toggle build_from_source?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 28, 2018

In general @MikeMcQuaid -- how does one use FormulaInstaller for rebuilding from source? Should I duplicate the logic from cmd/reinstall and just toggle build_from_source?

@amyspark Yeh, pretty much, thanks!

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 28, 2018

Done @MikeMcQuaid, thanks for your comments! Also, Codecov thinks this may need some tests 👇 -- does anyone know where to get or break test formulae for this purpose?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 29, 2018

Done @MikeMcQuaid, thanks for your comments! Also, Codecov thinks this may need some tests 👇 -- does anyone know where to get or break test formulae for this purpose?

@amyspark Check the existing tests; you can use formula do to specify new formulae for such things.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 30, 2018

@MikeMcQuaid:

  • Messaging was polished. odebug wasn't used anywhere in the Homebrew side, so I copied how upgrade lists the formulae.
  • The reinstallation logic was refactored into reinstall.rb, which upgrade and reinstall now require.
  • Build from source is only forced for poured kegs (k.tab.poured_from_bottle). Forcing this on kegs that were already built from source causes to unnecessarily download and build the whole dependency tree from source.
Show resolved Hide resolved Library/Homebrew/cmd/reinstall.rb Outdated
Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated
Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated
Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated
Show resolved Hide resolved Library/Homebrew/reinstall.rb
@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

This is looking really good! @reitermarkus, any thoughts?

Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated
Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated
Show resolved Hide resolved Library/Homebrew/cmd/upgrade.rb Outdated
@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 31, 2018

Ty @MikeMcQuaid! Shall I squash these too?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 31, 2018

@amyspark Up to you. I'd still like to wait and see what @reitermarkus thinks first but if he has no comments by e.g. Monday we could merge anyway.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 2, 2018

I have some mild concerns here around this squashing our ability to do things piecemeal. I merged in a hdf5 update the other day, for example, because it had a fairly weighty number of security fixes & everything except one of the brew uses was building okay. I essentially temporarily sacrificed the one formula for the "greater good".

Presumably, unless I'm misunderstanding (which is possible based on the amount of time I've had to look this over properly, sorry) if I do that after this everyone's going to get that one forced reinstallation from source locally and a good chunk of those will end up reporting it to us? Essentially my choice would become sit on security fixes for days or weeks after they've been publicly disclosed, or face being whomped by people filing issues for a known problem; is that the case?

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 2, 2018

I was initially testing this PR by checking out an old version of both homebrew/core and my osrf/simulation and then install the old protobuf 3.5.1 and something that uses protobuf like ignition-msgs1. Then I checkout the HEAD of core and brew upgrade protobuf. This pours the protobuf 3.6.0 bottle and then detects a linkage problem with ignition-msgs1 and rebuilds ignition-msgs1 from source. In this case, that works fine because the latest bottle of ignition-msgs1 had already been installed and it was bad, so a rebuild from source fixed things.

I think the logic in this PR as of cf9d84e isn't ideal for other scenarios though. For example, when boost 1.68.0 eventually lands ( Homebrew/homebrew-core#30914 ), it will introduce new bottles for boost and its dependents, such as boost-bcp. With this PR, running brew upgrade boost boost-bcp would pour boost from a bottle, then detect a linkage problem with boost-bcp while still processing boost and then build boost-bcp from source even though it has a good bottle available.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 2, 2018

@DomT4 This will only try to brew reinstall currently installed, newest version kegs with broken linkage on upgrade. I see this as improving rather than detracting from the situation you mention because it means those people won't have their stuff broken, need to notice and reinstall manually. I can't see why anyone will be filing issues for that as it is intended behaviour and will result in a brew upgrade or brew upgrade foo that doesn't end up randomly breaking all packages with opportunistic or optional linkage. This is a change of behaviour and may result in brew upgrade taking longer in some cases but, like when brew install fails to pour a bottle so tries to build from source, I see this as introducing a more correct behaviour. Please feel free to disagree though; I may have misunderstood your use-case.

With this PR, running brew upgrade boost boost-bcp would pour boost from a bottle, then detect a linkage problem with boost-bcp while still processing boost and then build boost-bcp from source even though it has a good bottle available.

@scpeters Very good spot/catch, here. That's indeed a problem that should be addressed before this is merged but shouldn't be too hard; we've already calculated the formulae_to_install so we could either check the one we're reinstalling isn't in that list or (my preference) only check the linkage after all the installs have been completed (so a separate loop through formulae_to_install after the upgrades have been completed).

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Sep 2, 2018

@scpeters -- good catch! Fix incoming.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 2, 2018

I see this as improving rather than detracting from the situation you mention because it means those people won't have their stuff broken, need to notice and reinstall manually.

Well, my point was more that sometimes we intentionally break stuff for the greater good. shogun currently has broken linkage, for example, but that can't be resolved until we verify & fix the checksum mismatch, which users would also see if they tried to brew reinstall shogun. That's the sort of situation I was vaguely trying to ask about.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 2, 2018

Well, my point was more that sometimes we intentionally break stuff for the greater good. shogun currently has broken linkage, for example, but that can't be resolved until we verify & fix the checksum mismatch, which users would also see if they tried to brew reinstall shogun. That's the sort of situation I was vaguely trying to ask about.

Hmm, yeh, I'm not sure how best to deal with this. I don't think it's worth blocking the feature on but I would like to think about something to deal with that. Even with shogun I think there's other options to consider other than breaking it; can we upload a mirror from the cache or something?

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 3, 2018

I don't think it's worth blocking the feature on but I would like to think about something to deal with that

Yeah, agreed. I'm a firm 👍 on the overall feature; just slightly concerned about either a) loss of flexibility in core or b) a hundred people filing or commenting on issues to tell me/us something I/we already know to be the case.

can we upload a mirror from the cache or something?

shogun is actually a pretty weird case. The thing that broke was a patch URL, using ?full_index=1. I haven't talked to upstream yet because there's a lot going on with brew at the moment & it all needs a lot of attention heh, but the only thing that I can think of that could possibly cause that would be if the repo was force pushed over, but I'm not 100% on that even.

I don't think it has been through CI recently enough for the prior known-good patch to have been retained in the cache.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 3, 2018

a hundred people filing or commenting on issues to tell me/us something I/we already know to be the case.

In this case it's only on 318 installs in 365 days so I think it's pretty safe. If loads of people comment on this stuff I think it's just a sign we need to prioritise it or remove the formula entirely. Ultimately this is only automating what people will do when they notice shogun is broken anyway.

the only thing that I can think of that could possibly cause that would be if the repo was force pushed over, but I'm not 100% on that even.

Nope, the SHA-1 would have changed then. It's definitely a change on GitHub's end. Honestly I'd just confirm the upstream commit still applies, update the checksum and move on. This isn't in upstream's control (it's effectively an undocumented/unsupported GitHub API) and a malicious actor couldn't change it even if they wanted to. It could be reported to GitHub at most but I'm pretty sure you'll be told "this isn't a supported API".

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 3, 2018

@amyspark I've just thought of another edge case that may want addressed if it's not already: the broken linkage may be fixed by an upgrade so we should check that the formula isn't outdated before we do a reinstall (because then you could do an upgrade instead).

Sorry there's so much back and forth here; this is a pretty complicated feature but it will be one that people will be extremely grateful for!

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 11, 2018

My first test is successful; I'm running one more and then I'll be ready to approve.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 11, 2018

Based on my second test, I think there is an edge case that is not handled properly. I think it's worth discussing, but I'm not sure if it's worth blocking over.

I think that the reinstallable broken dependents will be reinstalled in alphabetical order rather than topological order. For example, if I have 2 packages named a, b in a tap that both need to be reinstalled due to broken linkage, in my testing a will always be reinstalled before b even if a has depends_on "b" in its metadata.

Outside of that edge case, I'm ready to approve.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Sep 11, 2018

I think this is Set storing them in that order. Do you want me to whip up a sort code block so that we get alphabetical order for non-dependents, and then topological?

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 11, 2018

I think this is Set storing them in that order. Do you want me to whip up a sort code block so that we get alphabetical order for non-dependents, and then topological?

If it's not too much trouble; I think it would be worth testing. Thanks 😁

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Sep 11, 2018

@scpeters There you go 👍

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 11, 2018

quick work! testing now...

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 12, 2018

Hm... the upgrade command is aborting during the reinstallable.sort! block and it fails to reinstall any of the broken formulae. So it's a bit broken by 8135d06.

Unless @MikeMcQuaid or someone else has a good idea on how to sort these, then I think we should revert
8135d06 and open an issue describing the edge case.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Sep 12, 2018

@scpeters, I've reverted the sort stuff so that it does not break your testing. Could you please send me the stacktrace so I can have a look?

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 12, 2018

I can't get a stacktrace! It just exits, and strangely prints some Caveats again. I pasted the results of my hacking session into this gist:

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Sep 12, 2018

@scpeters It was the use of return inside the block (I don't seem to remember that keyword works differently in Ruby). You can try again now.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 12, 2018

Thanks for all the testing @scpeters and coding @amyspark!

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 12, 2018

@amyspark you've done it! This looks great to me now. Thanks for all your hard work!

Thanks for the extra reviews @MikeMcQuaid !

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 12, 2018

I guess we're missing some test coverage

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Sep 12, 2018

@MikeMcQuaid I'll flatten this now!
@scpeters I've not included tests as Homebrew only uses dummy formulae in its test suite.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 12, 2018

I've not included tests as Homebrew only uses dummy formulae in its test suite.

If I could find the time, I would try to create some dummy formulae with dependencies on each other. I'm not sure how to mock broken linkage in the dummy formulae, but I imagine that could be done as well.

Upgrade: implement linkage repair
After upgrading existing kegs, we now search and upgrade their
dependents as well. If any are detected that have broken linkage, they
are reinstalled from source.

If there are any formulae in the dependents tree that are pinned, they
are only reinstalled if they're not outdated; in all cases, a suitable
message is printed detailing the kegs that will be acted upon.

@amyspark amyspark changed the title upgrade: reinstall kegs w/broken linkage Upgrade: implement linkage repair Sep 12, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 12, 2018

I think this can go in without more test coverage; I trust @scpeters manual testing on this. Once we've got a final revision that both @amyspark and @scpeters are 👍 on being merged I'll 🚢 this!

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Sep 12, 2018

I'm 👍 for 🚢

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Sep 12, 2018

@MikeMcQuaid MikeMcQuaid merged commit 7c05612 into Homebrew:master Sep 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 13, 2018

Great work here @amyspark and nice work @scpeters for all the testing here 👏

@amyspark amyspark deleted the amyspark:upgrade-relink branch Sep 14, 2018

@lock lock bot added the outdated label Oct 14, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Oct 14, 2018

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