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.

Sign up for GitHub

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

Jump to bottom

bottle: tag specific cellars #10186

Merged
merged 1 commit into from
Jan 19, 2021
Merged

bottle: tag specific cellars #10186

merged 1 commit into from
Jan 19, 2021

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Dec 30, 2020

First draft implementation, opening the PR to show what I have already tested. I will need your help, this change is tricky.
See #9315 for discussion.

  • I know that some tests are broken and the implementation is not ironed out
  • I did not work on the brew bottle --merge --keep-old part
  • I know that we will need to split this PR in two parts:
    • first the reading of the old and new syntax
    • create a brew release
    • add the writing part

Writing part (brew bottle --merge):
I did not modify the bottle.json file content. This means I need to shift the cellar information from one level to another in the bottle hash. In fact, each bottle.json file has it's own cellar information, but it is not stored at the right level. So I had to manually merge the bottle hash between the multiple bottle.json files.
We might investigate a change in the bottle.json file format but I think we can do this in a second step.

Reading part (BottleChecksum):
I had to introduce a new BottleChecksum class. It's and adapted version of the Checksum class. I think I need to make BottleChecksum a subclass of Checksum.

@BrewTestBot
Copy link
Member

Review period will end on 2020-12-31 at 21:07:24 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 30, 2020
MikeMcQuaid
MikeMcQuaid reviewed Dec 31, 2020
View reviewed changes
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 so far. Does this break the existing DSL or are both types supported (they'll need to be indefinitely). Thanks!

Library/Homebrew/bottle_checksum.rb Outdated Show resolved Hide resolved
Library/Homebrew/bottle_checksum.rb Outdated Show resolved Hide resolved
Library/Homebrew/bottle_checksum.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bottle.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bottle.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bottle.rb Outdated Show resolved Hide resolved
Library/Homebrew/extend/os/mac/utils/bottles.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb 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 Jan 1, 2021
@iMichka
Copy link
Member Author

iMichka commented Jan 1, 2021

I reworked the PR to remove the writing part of the cellar lines. This part will be done in a second PR.
I also made the BottleChecksum a child class of Checksum, which simplifies the code.

I'll work on adding tests for the new syntax.

@iMichka iMichka marked this pull request as ready for review January 1, 2021 22:46
@iMichka iMichka requested a review from MikeMcQuaid January 1, 2021 22:46
@iMichka
Copy link
Member Author

iMichka commented Jan 1, 2021

This is ready for a new review, I added new tests too.

MikeMcQuaid
MikeMcQuaid reviewed Jan 5, 2021
View reviewed changes
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.

Nice work, getting there!

Library/Homebrew/extend/pathname.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
@iMichka iMichka force-pushed the bottle-cellar branch 2 times, most recently from 799ff24 to 25a3fe5 Compare January 6, 2021 10:14
MikeMcQuaid
MikeMcQuaid requested changes Jan 6, 2021
View reviewed changes
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.

The repeated use of .first is a bit concerning because we're effectively throwing away other results and it's not clear why they are being used.

Library/Homebrew/bottle_checksum.rb Outdated Show resolved Hide resolved
Library/Homebrew/bottle_checksum.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
@iMichka iMichka mentioned this pull request Jan 7, 2021
Merged
8 tasks
@iMichka
Copy link
Member Author

iMichka commented Jan 15, 2021

Finally got the CI green after 93 comments 😁 I’ll start working on the final comments and cleanup asap.

MikeMcQuaid
MikeMcQuaid reviewed Jan 18, 2021
View reviewed changes
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.

Looking good! One comment to be addressed and I'm happy.

@iMichka iMichka force-pushed the bottle-cellar branch 3 times, most recently from 627e1aa to 72a449a Compare January 18, 2021 17:22
@iMichka
Copy link
Member Author

iMichka commented Jan 18, 2021

Here we go, all cleaned up.

MikeMcQuaid
MikeMcQuaid approved these changes Jan 19, 2021
View reviewed changes
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.

Final change and I'm 👍🏻 to 🚢. Ensure you do so at a time when you can be around to make fixes if anything in CI bottling goes 💥

Library/Homebrew/test/formula_installer_bottle_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/formula_installer_bottle_spec.rb Outdated Show resolved Hide resolved
@iMichka iMichka merged commit 1c027f9 into Homebrew:master Jan 19, 2021
@iMichka iMichka deleted the bottle-cellar branch January 19, 2021 16:05
@iMichka iMichka mentioned this pull request Jan 20, 2021
Closed
8 tasks
iMichka added a commit to iMichka/brew that referenced this pull request Jan 24, 2021
@iMichka
cellars: write tag specific cellars
1f104d8 
With Homebrew#10186 now merged, the tag specific cellar information is being read by brew.
This PR (once merged) will start adding the cellar information for each tag instead
of having a single cellar line on the top of the bottle block.

Each new CI build in homebrew-core will slowly start migrating the cellar lines to
the right place. If keep-old is used, the old "all tag" cellar line is removed and
added to each tag.
@iMichka iMichka mentioned this pull request Jan 25, 2021
Closed
11 tasks
MikeMcQuaid pushed a commit to iMichka/brew that referenced this pull request Jan 28, 2021
@iMichka @MikeMcQuaid
cellars: write tag specific cellars
de2e309 
With Homebrew#10186 now merged, the tag specific cellar information is being read by brew.
This PR (once merged) will start adding the cellar information for each tag instead
of having a single cellar line on the top of the bottle block.

Each new CI build in homebrew-core will slowly start migrating the cellar lines to
the right place. If keep-old is used, the old "all tag" cellar line is removed and
added to each tag.
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 19, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BrewTestBot BrewTestBot BrewTestBot approved these changes

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

@sjackman sjackman Awaiting requested review from sjackman

@reitermarkus reitermarkus Awaiting requested review from reitermarkus

@dawidd6 dawidd6 Awaiting requested review from dawidd6

@SeekingMeaning SeekingMeaning Awaiting requested review from SeekingMeaning

Assignees

@iMichka iMichka

Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iMichka @BrewTestBot @SeekingMeaning @MikeMcQuaid