Skip to content
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

docs: document CI versions of Ubuntu #13625

Merged
merged 1 commit into from Aug 7, 2022
Merged

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Jul 31, 2022

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@BrewTestBot
Copy link
Member

Review period will end on 2022-08-02 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 31, 2022
@iMichka iMichka marked this pull request as draft July 31, 2022 21:48
@iMichka iMichka requested a review from MikeMcQuaid July 31, 2022 21:48
@iMichka
Copy link
Member Author

iMichka commented Jul 31, 2022

As asked in #13619, here is a first attempt to document our decision (if everyone is in line with this). The document is still a draft.

@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Aug 1, 2022
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks! Can we also add a maintainer guide for migrating to a new distribution?

docs/Linux-CI.md Outdated Show resolved Hide resolved
docs/Linux-CI.md Outdated Show resolved Hide resolved
docs/Linux-CI.md Outdated Show resolved Hide resolved
docs/Linux-CI.md Show resolved Hide resolved
docs/Linux-CI.md Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great so far! Thanks for this @iMichka!

docs/Linux-CI.md Outdated Show resolved Hide resolved
docs/Linux-CI.md Outdated Show resolved Hide resolved
docs/Linux-CI.md Outdated Show resolved Hide resolved
@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 2, 2022
@iMichka
Copy link
Member Author

iMichka commented Aug 3, 2022

I rewrote the documentation based on all your feedback.

@MikeMcQuaid I did not describe the migration process: I plan to do this in a second pull request, to keep the discussion focused in this one.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Great work @iMichka. Some optional suggestions. Happy for this to be merged with or without my suggestions once it's non-draft.

docs/Linux-CI.md Outdated Show resolved Hide resolved
docs/Linux-CI.md Outdated Show resolved Hide resolved
@iMichka
Copy link
Member Author

iMichka commented Aug 3, 2022

Done. Let's gather some more feedback before merging this.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Agree with the overall shape of this, but perhaps some of the details need working out.

docs/Linux-CI.md Outdated Show resolved Hide resolved
docs/Linux-CI.md Outdated Show resolved Hide resolved
@iMichka
Copy link
Member Author

iMichka commented Aug 3, 2022

@MikeMcQuaid / @carlocab I initially wrote 22.04, and I think this was discussed elsewhere, but I thought no strong decision was taken up to now.
I sort of changed my mind as I wrote the text, because I wanted an explanation for our reasoning. It's my fault that it changed in this pull request.

I was looking at the statistics @sjackman posted on Slack: 50% of our users are using Ubuntu 20.04 and 20% Ubuntu 22.04.

This means that if we use 22.04, more than 50% of our users will need to install Homebrew's glibc.

My logic for using 20.04 (the proposed latest LTS - 1 "rule") was that there is a 2 year timeframe after a LTS release were adoption is low. Using the latest version as soon as it is released is probably not ideal. With this strategy it leaves 2 years for usage to grow.

On macOS there are always 3 supported versions: the rotation is 3 years. There we can always ship the latest version because we build for 3 macOS versions. On Linux we are not doing this, so a 2 year rotation is a good middle ground in my opinion.

Of course if we already decided to use 22.04 and there are good reasons (that we want to write down), then disregard my change and let me know :)

@danielnachun
Copy link
Member

I'll try to make a case for 22.04 instead. From what I understand, the desire to limit how many users have to use brewed glibc and seems to have at least 3 motivations:

  1. Extra disk space usage - my response to this is that disk space is cheap and we have much larger formulae like LLVM which are widely installed.
  2. For non-default prefixes, glibc is trickier to install because it has to use the host toolchain - now that we are (for now) vendoring all but the most basic build dependencies in glibc 2.35 homebrew-core#106837 and I hope to get glibc an :any cellar soon, this concern should be greatly reduced and hopefully irrelevant in the future.
  3. For non-default prefixes, some formulae may fail to build with brewed glibc that work if the host glibc were being used - this invariably seems to be result of shim bypasses or opportunistic linking, which is something that formula aren't supposed to be doing and should be fixed. If binary patching actually succeeds in a 95% reduction in non-relocatable formulae I'm anticipating from previous testing, this will also become a largely irrelevant concern.

There is also concern about using a newer GCC in 22.04 in CI because some (mostly C++) formulae will fail to build, but I also think this is something we should try to fix, and usually it's just a matter of some missing include statements. It's also trivial to build with an older brewed GCC if needed, because at run time the libstdc++ provided by the either the host or brewed GCC is backwards compatible and consequently the older GCC only has to be build dependency.

By contrast, when a formula needs a newer GCC because our host GCC in CI is too old, it can be quite painful. All C++ dependents of that formulae immediately acquire a dependency on brewed GCC as well. While we have taken the steps to make sure this no longer holds up GCC updates, it still creates a maintenance headache. Importantly, unlike the above issues, it is not due to "bad behavior" on the part of the formula like shim bypasses, opportunistic linking or lack of maintenance to maintain compatibility with newer GCC.

Ironically this problem is more likely for formula which are very actively maintained and try to use newer features of C++. To me we shouldn't be creating maintenance burdens for formulae which are doing the right thing by staying up to date. It makes a lot of sense for us to be submitting upstream fixes when formulae are missing include statements or hardcoding compilers. It makes a lot less sense for us to be spending time playing "catch up" with the latest C++ standards because our host compiler is too old.

There are probably many valid rebuttals to what I've said so please let me know what you think!

@MikeMcQuaid
Copy link
Member

My logic for using 20.04 (the proposed latest LTS - 1 "rule") was that there is a 2 year timeframe after a LTS release were adoption is low. Using the latest version as soon as it is released is probably not ideal. With this strategy it leaves 2 years for usage to grow.

Does this mean we have a 2 year window to migrate but also we have to migrate in that 2 year window?

I'd probably like to see something like e.g. we use the latest LTS at least ~3-4 months after release and we always migrate to the latest LTS within at most 12 months of release.

Given we're a rolling-release package manager and we're aggressively migrating to the latest macOS version on release (sometimes even in public beta): we need to ensure that macOS and Linux are somewhat in-step with e.g. compiler usage.

This means that if we use 22.04, more than 50% of our users will need to install Homebrew's glibc.

  1. my response to this is that disk space is cheap

I agree with this. This is similar to what we've done with portable Ruby on macOS: often only the newest version of macOS with relatively low adoption avoids a portable Ruby but this makes it easier for us to be able to rely on a newer Ruby version everywhere. Same deal here but with glibc and gcc.

@danielnachun for reference: how big is a 1) bottled and 2) installed glibc today?

2. I hope to get glibc an :any cellar soon, this concern should be greatly reduced and hopefully irrelevant in the future.

@danielnachun how hard is this looking currently? Is it a question of overzealous detection we could hack around in Homebrew/brew or legitimate location hardcoding that will break when moved?

To me we shouldn't be creating maintenance burdens for formulae which are doing the right thing by staying up to date.

Agreed. Again: we're a rolling-release package manager. We try to ship the newest things as quickest as possible. That's going to be easier for us on Linux with newer toolchains.

@iMichka
Copy link
Member Author

iMichka commented Aug 4, 2022

I rewrote the document for Ubuntu 22.04, with some more explanations. @danielnachun / @MikeMcQuaid I copied / adjusted some stuff you said above directly into the documentation. It will probably need some rewording but at least we can all agree on the reasoning and the final choice for Ubuntu 22.04 (this year) and the proposed migration timeline.

@danielnachun
Copy link
Member

@danielnachun for reference: how big is a 1) bottled and 2) installed glibc today?

It's 200MB in both cases to my knowledge. Not the smallest package but not huge either.

@danielnachun how hard is this looking currently? Is it a question of overzealous detection we could hack around in Homebrew/brew or legitimate location hardcoding that will break when moved?

It is absolutely legitimate location hardcoding, so prefix patching is obligatory for this to work. That on its own isn't too bad as we know how we will implement that. The harder part is that there are 3 issues I'm aware of where prefix patching is insufficient to make glibc relocatable.

Two of them are because in iconv and gconv there are string constants with prefixes in them that are "garbled" in such a way that we can't do the standard replacement and the tools end up failing because they still have prefix references. I suspect this is because of how the constants are defined in the code but I have to test if that is actually right.

The other problem is that ld.so appears to bake in a list of directories to search for libraries separated by of all things a null character, which gets messed up when the prefix replacement occurs. I believe it should be possible to write and upstream a patch to fix these issues, but I haven't had the time to dig into this yet because of all the other issues we're trying to work out right now. For that reason I'd like to tackle this once we've completed the migration, as I can't promise a timeline of when I'll be able to figure everything out.

@MikeMcQuaid
Copy link
Member

@danielnachun for reference: how big is a 1) bottled and 2) installed glibc today?

It's 200MB in both cases to my knowledge. Not the smallest package but not huge either.

Yeh, that's tiny.

It is absolutely legitimate location hardcoding, so prefix patching is obligatory for this to work. That on its own isn't too bad as we know how we will implement that. The harder part is that there are 3 issues I'm aware of where prefix patching is insufficient to make glibc relocatable.

@danielnachun Good to know. From my perspective I think we need to operate with the assumption that this will take a non-trivial amount of time after a migration to occur. If we beat that pessimistic assumption: great, but let's not rely on it for our plans.

Does that change anyone's mind given it's the case?

For that reason I'd like to tackle this once we've completed the migration, as I can't promise a timeline of when I'll be able to figure everything out.

👍🏻 totally fair @danielnachun.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Wording is great.

@iMichka
Copy link
Member Author

iMichka commented Aug 7, 2022

Let's merge this. I'll work on adding a small section for the necessary migration steps (though these may change over time as new challenges arise).

@iMichka iMichka merged commit 402e267 into Homebrew:master Aug 7, 2022
@iMichka iMichka deleted the ubuntu branch August 7, 2022 20:29
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 7, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants