Skip to content

astgen 3.4.0#139200

Merged
BrewTestBot merged 2 commits intoHomebrew:masterfrom
chenrui333:bump-astgen-3.4.0
Aug 11, 2023
Merged

astgen 3.4.0#139200
BrewTestBot merged 2 commits intoHomebrew:masterfrom
chenrui333:bump-astgen-3.4.0

Conversation

@chenrui333
Copy link
Copy Markdown
Member

Created by brew bump


Created with brew bump-formula-pr.

Details

release notes

@github-actions github-actions bot added nodejs Node or npm use is a significant feature of the PR or issue bump-formula-pr PR was created using `brew bump-formula-pr` labels Aug 10, 2023
@github-actions
Copy link
Copy Markdown
Contributor

🤖 An automated task has requested bottles to be published to this PR.

@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 Aug 10, 2023
@BrewTestBot BrewTestBot enabled auto-merge August 10, 2023 14:31
@ZhongRuoyu
Copy link
Copy Markdown
Member

Needs rebase after #138921

Comment on lines 11 to 17
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks strange 🤔

Copy link
Copy Markdown
Member

@ZhongRuoyu ZhongRuoyu Aug 10, 2023

Choose a reason for hiding this comment

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

CC @Homebrew/brew

It seems that we no longer have an all bottle. I suspect it's related to sharding?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ZhongRuoyu not sure what's happening here. @carlocab may have an idea. doubt sharding is to blame as the checksums are the same and the repo is the same. we've only recently fixed the previously broken all bottle situation Homebrew/brew#15819

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't tell of the top of my head, but I'll have a look.

Copy link
Copy Markdown
Member

@carlocab carlocab Aug 11, 2023

Choose a reason for hiding this comment

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

Generating an :all bottle comes from the logic here:

all_bottle = !args.no_all_checks? &&
             (!old_bottle_spec_matches || bottle.rebuild != old_bottle_spec.rebuild) &&
             tag_hashes.count > 1 &&
             tag_hashes.uniq { |tag_hash| "#{tag_hash["cellar"]}-#{tag_hash["sha256"]}" }.count == 1

Running

brew bottle \
  --merge \
  --write \
  --verbose \
  --debug \
  astgen--3.4.0.x86_64_linux.bottle.json \
  astgen--3.4.0.ventura.bottle.json \
  astgen--3.4.0.arm64_ventura.bottle.json \
  astgen--3.4.0.monterey.bottle.json \
  astgen--3.4.0.big_sur.bottle.json \
  astgen--3.4.0.arm64_monterey.bottle.json \
  astgen--3.4.0.arm64_big_sur.bottle.json

which is the command that brew pr-pull uses to generate the bottle block (via brew pr-upload) has all_bottle evaluating to false for some reason. The bit that is making all_bottle evaluate this way is

(!old_bottle_spec_matches || bottle.rebuild != old_bottle_spec.rebuild)

(the other conditions are true).

I'm still working out what this condition is meant to check for, but it seems to be wrong for this formula for some reason.

Copy link
Copy Markdown
Member

@Bo98 Bo98 Aug 11, 2023

Choose a reason for hiding this comment

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

That line is to make sure new dispatched keep-old bottles (i.e. bottles that don't trigger a new version or a rebuild) don't add an all bottle.

Part of that condition (see old_bottle_spec_matches) is a pkg_version comparison. So if that's not evaluating, then that must mean the "old bottle spec" we are loading isn't actually the old bottle spec. (This doesn't happen with new formulae, because new formulae don't have a bottle block at all.)

The problem is because of sharding, but probably wouldn't have happened with a history rewrite. Basically FormulaVersions works based on paths, so the formula before the sharding move is not recognised as the same formula as a formula after the sharding move.

brew update-report has similar issues, but in separate code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Bo98 and @carlocab! @Bo98 could you open issue(s) for this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That line is to make sure new dispatched keep-old bottles

Shouldn't it check for --keep-old then?

Copy link
Copy Markdown
Member

@Bo98 Bo98 Aug 11, 2023

Choose a reason for hiding this comment

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

Shouldn't it check for --keep-old then?

Maybe, but there's also several other things we use old_bottle_spec for that's equally broken.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Bo98 and @carlocab! @Bo98 could you open issue(s) for this?

Poorly written and not really following the template properly but here it is: Homebrew/brew#15856 (please edit it to clean it up where needed)

@ZhongRuoyu ZhongRuoyu added the ready to merge PR can be merged once CI is green label Aug 10, 2023
@chenrui333

This comment was marked as resolved.

@carlocab
Copy link
Copy Markdown
Member

Let's merge this now and see if rebottling will help.

@BrewTestBot BrewTestBot added this pull request to the merge queue Aug 11, 2023
Merged via the queue into Homebrew:master with commit 799d946 Aug 11, 2023
@chenrui333 chenrui333 deleted the bump-astgen-3.4.0 branch August 11, 2023 15:05
@carlocab
Copy link
Copy Markdown
Member

Rebottling seems to have helped. Checksum didn't even change: c940d78

@Bo98
Copy link
Copy Markdown
Member

Bo98 commented Aug 11, 2023

Yeah the issue will only happen for the first bottle post-sharding move.

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 11, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bump-formula-pr PR was created using `brew bump-formula-pr` CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. nodejs Node or npm use is a significant feature of the PR or issue outdated PR was locked due to age ready to merge PR can be merged once CI is green

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants