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

icu4c 73.2 #133611

Merged
merged 2 commits into from Jun 14, 2023
Merged

icu4c 73.2 #133611

merged 2 commits into from Jun 14, 2023

Conversation

carlocab
Copy link
Member

Cherry-picked from #128294 for merging into an icu4c-staging branch.

@carlocab carlocab added the CI-skip-dependents Pass --skip-dependents to brew test-bot. label Jun 14, 2023
@carlocab carlocab mentioned this pull request Jun 14, 2023
@carlocab carlocab added the maintainer feedback Additional maintainers' opinions may be needed label Jun 14, 2023
@carlocab
Copy link
Member Author

We've been stuck on #128294 for a while, and there have been multiple issues filed about the delay, I want to use a staging branch to try to speed up getting this merged to master while limiting the struggle with long-running PRs.

The idea is that we merge this with bottles onto icu4c-staging, and then open follow-up PRs for the dependents, where each PR also targets icu4c-staging as the base branch.

Once all the dependents have been handled, we can merge icu4c-staging into master.

CC @Homebrew/core

@carlocab carlocab added the automerge-skip `brew pr-automerge` will skip this pull request label Jun 14, 2023
@carlocab carlocab requested review from a team and MikeMcQuaid as code owners June 14, 2023 10:21
@github-actions github-actions bot added the workflows PR modifies GitHub Actions workflow files label Jun 14, 2023
@carlocab
Copy link
Member Author

Lol. Oops.

@carlocab carlocab closed this Jun 14, 2023
@carlocab carlocab deleted the icu4c-73.1 branch June 14, 2023 10:23
@carlocab carlocab restored the icu4c-73.1 branch June 14, 2023 10:23
@carlocab carlocab reopened this Jun 14, 2023
@github-actions github-actions bot removed the automerge-skip `brew pr-automerge` will skip this pull request label Jun 14, 2023
@carlocab carlocab added the automerge-skip `brew pr-automerge` will skip this pull request label Jun 14, 2023
@carlocab carlocab changed the title icu4c 73.1 icu4c 73.2 Jun 14, 2023
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.

Makes sense to me!

We may want to enable some branch protections for icu4c-staging or *-staging branches. They should be different to those for master.

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

⚠️ @carlocab bottle publish failed. CC @carlocab

@github-actions
Copy link
Contributor

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Jun 14, 2023
@BrewTestBot BrewTestBot merged commit 979fae0 into Homebrew:icu4c-staging Jun 14, 2023
6 checks passed
@carlocab carlocab deleted the icu4c-73.1 branch June 14, 2023 15:21
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Jun 14, 2023
@carlocab
Copy link
Member Author

We may want to enable some branch protections for icu4c-staging or *-staging branches. They should be different to those for master.

Yes, agreed for *-staging branches. What branch protections do you suggest?

Some additional thoughts on staging branch use. I personally found it a lot easier than trying to work with a single giant PR, for a few reasons:

  • It was a lot easier to fix individual formulae, since that can be done in an isolated PR rather than along with all the other revision bumps. Unlike previous large PRs, we didn't ship this version of icu4c with the knowledge that it'll break a dependent.
  • The resulting CI logs are a lot easier to parse too, since you don't have a few dozen other formulae builds clogging up the log as well.
  • It was also nice to not have to worry about opportunistic linkage at all.

Some things that could be improved:

  • With a staging branch, it's not really clear what the system is for working out which dependents need revision bumps. This wasn't so much an issue for icu4c, since we only need to check which dependents have linkage, but this might be an issue for other formulae.
  • We need tooling to sort the dependents in the order they have to be rebuilt (i.e. a topological sort).
  • We could/should probably try to limit the number of merge commits that get added to the staging branch. This is more than just for trying to keep the git log clean: GitHub has different handling for PRs with a large number of commits (~1000), and it's not clear whether that change interacts correctly with our merge flow.

@MikeMcQuaid
Copy link
Member

Yes, agreed for *-staging branches. What branch protections do you suggest?

I'm not sure, I haven't followed closely how they are being used? Disable force-pushing? CI? Approvals? Note: these will only affect PRs being merged into the staging branch.

  • Unlike previous large PRs, we didn't ship this version of icu4c with the knowledge that it'll break a dependent.

This is a huge improvement.

I personally found it a lot easier than trying to work with a single giant PR

I would definitely be game to see them used more widely. I think if there's documentation and potentially some tooling improvements: they could become the default for anything with a complex dependent chain.

Another measure for "should this be a staging branch?" could be something like "is this a long build or not?". It feels like doing things all in one huge PR is not a big deal when the whole PR can be done inside the not-long-build window. Thoughts?

  • With a staging branch, it's not really clear what the system is for working out which dependents need revision bumps. This wasn't so much an issue for icu4c, since we only need to check which dependents have linkage, but this might be an issue for other formulae.

Yeh, ideally CI will catch/enforce this somehow (but not sure what/how).

  • We need tooling to sort the dependents in the order they have to be rebuilt (i.e. a topological sort).

Agreed. There's at least some topological sorting of dependencies in Homebrew/brew and/or Homebrew/homebrew-bundle and/or Homebrew/homebrew-test-bot that could perhaps be utilised or reused?

  • We could/should probably try to limit the number of merge commits that get added to the staging branch. This is more than just for trying to keep the git log clean: GitHub has different handling for PRs with a large number of commits (~1000), and it's not clear whether that change interacts correctly with our merge flow.

This seems reasonable. Another option (contradicting something I said above!) would be considering if rebasing is better than merge commits in this case or to rebase the final thing before it's ready for merge.

Alternatively, have some sort of programmatic limit for how to decide how many/when to do this.

Great work @carlocab, love to see this sort of innovation!

@github-actions github-actions bot added the outdated PR was locked due to age label Jul 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. CI-skip-dependents Pass --skip-dependents to brew test-bot. maintainer feedback Additional maintainers' opinions may be needed outdated PR was locked due to age workflows PR modifies GitHub Actions workflow files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants