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

update: migrate everyone from linuxbrew-core to homebrew-core #12248

Merged
merged 1 commit into from Oct 20, 2021

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Oct 18, 2021

  • 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 2021-10-19 at 17:30:52 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Oct 18, 2021
@iMichka
Copy link
Member Author

iMichka commented Oct 18, 2021

It has been 7 days since we enabled the migration for developers and we got no complaints. I would like to get this out as soon as possible, before we start massively bottling for Monterey (to avoid to have to merge all these changes into linuxbrew-core, which might be only endless merge conflicts in bottle blocks).

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.

I'm 👍🏻 on getting this out ASAP and tagging a minor or major release with this in the next week or two. Doesn't need to be in this PR but I think we should also enable GITHUB_ACTIONS sooner rather than later and adjust/remove the documentation for HOMEBREW_FORCE_HOMEBREW_ON_LINUX and HOMEBREW_FORCE_HOMEBREW_CORE_REPO_ON_LINUX to make it clear that they don't do anything now.

@iMichka
Copy link
Member Author

iMichka commented Oct 18, 2021

I think a few cleanup / decommissioning pull requests will be necessary anyway. In this repo and elsewhere. We can do this slowly after we have done the migration.

@MikeMcQuaid
Copy link
Member

@iMichka Agreed. I'd like GITHUB_ACTIONS to land sooner rather than later, though, because it's a large proportion of our usage.

Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
@iMichka
Copy link
Member Author

iMichka commented Oct 18, 2021

@MikeMcQuaid HOMEBREW_CORE_ON_LINUX=1 was set for both conditions of the if/else, so now that we migrate everyone I think it has become useless. I made a few more tweaks.

@iMichka
Copy link
Member Author

iMichka commented Oct 18, 2021

Interestingly this broke the syntax job: it's downloading from the right place (homebrew-core), but uses the sha256 from linuxbrew-core...

@MikeMcQuaid
Copy link
Member

@iMichka Yeh, that's somewhat expected because until brew update is run the homebrew-core repository will still be on linuxbrew-core (and auto-update is disabled there).

The right approach is probably to restore the previous logic and only use it when HOMEBREW_NO_AUTO_UPDATE is set. Eventually, we'll probably want to hard-error out when that is set and a migration is needed (so we can remove the necessary fallback code).

@Bo98
Copy link
Member

Bo98 commented Oct 19, 2021

I think we should also enable GITHUB_ACTIONS sooner rather than later

Instead of forcing a migration onto GitHub Actions, we really should update the default install remote in Homebrew/install and allow GitHub to ship new images that use this remote (since new image versions are clean built). That means migration will not be required at all from GitHub Actions.

@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 Oct 19, 2021
@iMichka
Copy link
Member Author

iMichka commented Oct 19, 2021

I reopened my install.sh pull request. Initially I wanted to merge that one first.

The docker image used by the CI is built by this dockerfile (https://github.com/Homebrew/brew/blob/master/Dockerfile), right?
It does not use install.sh, so not sure how this is going to change the images we ship.

@MikeMcQuaid
Copy link
Member

Instead of forcing a migration onto GitHub Actions, we really should update the default install remote in Homebrew/install and allow GitHub to ship new images that use this remote (since new image versions are clean built). That means migration will not be required at all from GitHub Actions.

We don't have control over when GitHub does that. We can ask but it's unlikely that they will before this is in a new tag. You'll also only end up on this commit if HOMEBREW_DEVELOPER and/or homebrew.devcmdrun are set.

The docker image used by the CI is built by this dockerfile (https://github.com/Homebrew/brew/blob/master/Dockerfile), right? It does not use install.sh, so not sure how this is going to change the images we ship.

Anything that doesn't reference Homebrew/linuxbrew-core specifically but does homebrew-core will change after this PR. This means these images will be updated but not for the tagged ones.

@XuehaiPan
Copy link
Contributor

Hi, I'm very glad to see this migration to let users have unified settings for Homebrew on both macOS and Linux. May I ask is there any migration plan for formula bottles (HOMEBREW_BOTTLE_DOMAIN)?

HOMEBREW_BOTTLE_DOMAIN: {
description: "Use this URL as the download mirror for bottles. " \
"If bottles at that URL are temporarily unavailable, " \
"the default bottle domain will be used as a fallback mirror. " \
"For example, `HOMEBREW_BOTTLE_DOMAIN=http://localhost:8080` will cause all bottles to " \
"download from the prefix `http://localhost:8080/`. " \
"If bottles are not available at `HOMEBREW_BOTTLE_DOMAIN` " \
"they will be downloaded from the default bottle domain.",
default_text: "macOS: `https://ghcr.io/v2/homebrew/core`, " \
"Linux: `https://ghcr.io/v2/linuxbrew/core`.",
default: HOMEBREW_BOTTLE_DEFAULT_DOMAIN,
},

HOMEBREW_CORE_GIT_REMOTE: {
description: "Use this URL as the Homebrew/homebrew-core `git`(1) remote.",
default_text: "macOS: `https://github.com/Homebrew/homebrew-core`, " \
"Linux: `https://github.com/Homebrew/linuxbrew-core`.",
default: HOMEBREW_CORE_DEFAULT_GIT_REMOTE,
},

@MikeMcQuaid MikeMcQuaid force-pushed the migrate2 branch 4 times, most recently from 467fa7d to 444b9b4 Compare October 19, 2021 07:57
@MikeMcQuaid
Copy link
Member

@XuehaiPan fixed!

@MikeMcQuaid
Copy link
Member

@Bo98 This will avoid the need for GitHub Actions migration where our workflow is used: Homebrew/actions#235

@iMichka We also want to merge that at a similar time to this and the install PR.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Oct 19, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Oct 19, 2021
@BrewTestBot
Copy link
Member

BrewTestBot commented Oct 19, 2021

Review period ended.

@iMichka
Copy link
Member Author

iMichka commented Oct 19, 2021

Tested and the migration is not done anymore:

$ docker run -it homebrew/ubuntu16.04
$ cd /home/linuxbrew/.linuxbrew/Homebrew
$ git remote add iMichka https://github.com/iMichka/brew
$ git fetch iMichka
$ git checkout migrate2
$ brew update
HOMEBREW_CORE_GIT_REMOTE set: using https://github.com/Homebrew/linuxbrew-core for Homebrew/core Git remote.
remote: Enumerating objects: 42040, done.
remote: Counting objects: 100% (19560/19560), done.
remote: Compressing objects: 100% (2501/2501), done.
remote: Total 42040 (delta 17245), reused 19369 (delta 17059), pack-reused 22480
Receiving objects: 100% (42040/42040), 14.15 MiB | 1.53 MiB/s, done.
Resolving deltas: 100% (32291/32291), completed with 1831 local objects.
From https://github.com/Homebrew/linuxbrew-core
   70d37801899..5be975b7335  master     -> origin/master
...
Migrating formulae from linuxbrew-core to homebrew-core
...

The remote stays on linuxbrew-core.

Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

I'm not sure if we actually need HOMEBREW_CORE_ON_LINUX anymore and I feel the complication of it is causing issues. @iMichka does the below fix anything?

Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/cmd/update.sh Outdated Show resolved Hide resolved
@Bo98
Copy link
Member

Bo98 commented Oct 19, 2021

Ah I just noticed you said this method broke the syntax job before? I think that might have actually been fixed when we updated setup-homebrew. The problem we had there happens if brew is updated but not via update.sh, but I'm not really sure if there's much we can do about that without supporting linuxbrew-core settings indefinitely?

Copy link
Member

@Rylan12 Rylan12 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 to me (although, again, I haven't tested it)! I can think of a few cleanup things that can be done now, but I don't know if they should be addressed in a follow up PR rather than now.

@@ -24,5 +24,3 @@ def fetch(category, days)
end
end
end

require "extend/os/api/analytics"
Copy link
Member

Choose a reason for hiding this comment

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

There's some more cleanup that can happen here later (e.g. no need for the analytics_api_path generic_analytics_api_path dance)

Happy to help out with this in the future. No need to worry in this PR since it should never be encountered from this point on.

Copy link
Member

Choose a reason for hiding this comment

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

@Rylan12 Yeh, let's punt on this cleanup until later.

Comment on lines +20 to +21
- Linux maintainers: this team maintains the [`Homebrew/homebrew-core`](https://github.com/Homebrew/homebrew-core)
repository on Linux.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Linux maintainers: this team maintains the [`Homebrew/homebrew-core`](https://github.com/Homebrew/homebrew-core)
repository on Linux.

Not sure we need to make a distinction here anymore. I figure all linux maintainers will just be considered core maintainers now.

Also, the Homebrew/linuxbrew-core Maintainer Guide can probably be deleted, though there may be some information there that should just be added to the formula cookbook (i.e. stuff about Linux-only formulae)

Copy link
Member

Choose a reason for hiding this comment

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

@Rylan12 I think it'd be good to have a subset of maintainers who can still get pinged on Linux issues.

Yeh, I'm holding off deleting that guide and will let one of the Linux maintainers do so.

Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Alternative fix to my above suggestion: adjust the logic so that it actually overrides HOMEBREW_CORE_GIT_REMOTE on migration.

@Bo98
Copy link
Member

Bo98 commented Oct 19, 2021

The right approach is probably to restore the previous logic and only use it when HOMEBREW_NO_AUTO_UPDATE is set. Eventually, we'll probably want to hard-error out when that is set and a migration is needed (so we can remove the necessary fallback code).

This should probably happen now rather than later, at least as a warning, if we are planning to archive linuxbrew-core immediately after this is released.

I also feel the migration experience isn't 100% ideal yet. You currently have to run brew update twice for the migration to occur.

If we can get it to trigger immediately after the update to this version of brew then we can have a lot less legacy handling here. To do that however will require making something like brew.sh/update-report.rb trigger a second brew update which is a little bit awkward.

I can see us doing something like:

  • If something like HOMEBREW_UPDATED is set in brew.sh (which should only happen if we are about to invoke brew update-report) but repo migration hasn't happened yet then trigger another brew update which will move migration to the next stage.
  • Add diagnostics blocking formula installation if linuxbrew-core is still the origin. If the above is implemented, this will only happen for manual update flows or brew update-reset.

The above may well stay permanently, but will at least be minimalised.

If we want to give a migration period then of course that diagnostic can be just a warning for now and we can keep the old bottle domains while we give notice.

@MikeMcQuaid MikeMcQuaid force-pushed the migrate2 branch 3 times, most recently from 8dfcd84 to 8b71cfa Compare October 20, 2021 10:00
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
@MikeMcQuaid
Copy link
Member

If we can get it to trigger immediately after the update to this version of brew then we can have a lot less legacy handling here. To do that however will require making something like brew.sh/update-report.rb trigger a second brew update which is a little bit awkward.

I've made update-report.rb do this.

  • Add diagnostics blocking formula installation if linuxbrew-core is still the origin. If the above is implemented, this will only happen for manual update flows or brew update-reset.

Have also added this.

@MikeMcQuaid
Copy link
Member

Tested and seems to be working as expected. Want to get this merged so we can start testing here before tagging this next week.

@MikeMcQuaid MikeMcQuaid merged commit 0cf4f9d into Homebrew:master Oct 20, 2021
@iMichka iMichka deleted the migrate2 branch October 20, 2021 17:20
@iMichka
Copy link
Member Author

iMichka commented Oct 20, 2021

Tested on my side, it worked. Thanks @MikeMcQuaid for all the cleanup! I feel free now :)

@github-actions github-actions bot added the outdated PR was locked due to age label Nov 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants