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

Overhaul Formula/Cask JSON generation #14615

Merged
merged 1 commit into from Feb 14, 2023

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Feb 13, 2023

Take a slightly hackier but more reliable method of setting up $HOMEBREW_PREFIX, $HOME and --appdir.

  • Use constants for placeholders
  • Monkeypatch to set HOMEBREW_PREFIX consistently to placeholder
  • Use environment variable to set Dir.home consistently to placeholder
  • Use appdir short-circuit to set Cask#appdir consistently to placeholder
  • Use Cask.generating_hash! to enable "generating mode" with these patches
  • Fix Formula#caveats from JSON

See Homebrew/formulae.brew.sh#744 for the formulae.brew.sh side.

Fixes #14505
Fixes #14595

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Feb 13, 2023
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

MikeMcQuaid added a commit to Homebrew/formulae.brew.sh that referenced this pull request Feb 13, 2023
See Homebrew/brew#14615

This will set placeholder values in the generated hash.
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.

Fantastic, thanks! Good we managed to get this in for 4.0.0 as I initially thought it might've been too late.

I think this covers the last of the known cask issues, which is excellent news.

The slight snag to be aware of is we might be breaking people until their next auto-update if we're generating the JSON with the new placeholders right away (same scenario as #14611).

Library/Homebrew/cask/cask.rb Outdated Show resolved Hide resolved
@Bo98 Bo98 added the install from api Relates to API installs label Feb 14, 2023
Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

In general, I think this is a good idea. A few thoughts...

I wonder what the performance implications are for this when calling brew info --json=v2 --eval-all though that's not something we need to necessarily figure out now.

It would be nice to see some regression tests here.

And like @Bo98 I'm a bit confused about this only being enabled by default in Cask#to_h when we use Cask#to_hash_with_variations to generate the API json.

Library/Homebrew/cask/cask.rb Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member Author

It would be nice to see some regression tests here.

Added.

And like @Bo98 I'm a bit confused about this only being enabled by default in Cask#to_h when we use Cask#to_hash_with_variations to generate the API json.

Removed from both, will put only in Homebrew/formulae.brew.sh#744 instead.

- Use constants for placeholders
- Monkeypatch to set `HOMEBREW_PREFIX` consistently to placeholder
- Use environment variable to set `Dir.home` consistently to placeholder
- Use `appdir` short-circuit to set `Cask#appdir` consistently to placeholder
- Use `Cask.generating_hash!` to enable "generating mode" with these patches
- Fix `Formula#caveats` from JSON

Fixes #14505
Fixes #14595
@MikeMcQuaid MikeMcQuaid merged commit 98f5213 into Homebrew:master Feb 14, 2023
@MikeMcQuaid MikeMcQuaid deleted the generating_hash_appdir branch February 14, 2023 15:02
MikeMcQuaid added a commit to Homebrew/formulae.brew.sh that referenced this pull request Feb 14, 2023
See Homebrew/brew#14615

This will set placeholder values in the generated hash.
MikeMcQuaid added a commit to Homebrew/formulae.brew.sh that referenced this pull request Feb 14, 2023
See Homebrew/brew#14615

This will set placeholder values in the generated hash.
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.

Thanks, @MikeMcQuaid! This looks great, sorry for the review delay!

MikeMcQuaid added a commit to Homebrew/formulae.brew.sh that referenced this pull request Feb 15, 2023
See Homebrew/brew#14615

This will set placeholder values in the generated hash.
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 18, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023
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. install from api Relates to API installs outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

staged_path in casks is parsed to wrong path Emacs install fails when appdir is set
5 participants