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

readall: check hash generation works #16053

Merged
merged 1 commit into from Sep 28, 2023

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Sep 28, 2023

Rationale is that we could swap brew generate-formula-api (60-90s) with this (15s) addition to brew readall.

If to_hash fails, there is likely a typo or other sort of error in the formula that would render various brew operations non-functional on that formula even among things that don't call to_hash, e.g. brew info or simply brew install, alongwith API generation itself.

In theory, a passing brew readall should now guarantee API generation works (excluding brew bugs in the command itself of course, but brew CI should continue to run brew generate-formula-api). A "readable" formula should mean you can extract all the information about the formula.

@Bo98
Copy link
Member Author

Bo98 commented Sep 28, 2023

See https://github.com/Homebrew/homebrew-core/actions/runs/6339913183/job/17220138593 for the latest timings after the readall and audit perf improvements. brew generate-formula-api is now the slowest part of it. This PR will bring brew readall close to the same timings as brew generate-formula-api, because it'll largely be doing the same thing. But we can then remove the cost of loading formula files twice by dropping the API generation command, for an overall speed benefit for hopefully close to zero risk.

~90s (or at least less than 2 minutes) should be ok to run in PRs again to avoid confusions like seen in Homebrew/homebrew-core#139538, and also saves precious self-hosted CI time on things that were going to fail audits anyway.

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.

Yup, great work again @Bo98, makes sense to me and I agree with the proposed CI changes along with keeping the actual generation in Homebrew/brew.

@MikeMcQuaid MikeMcQuaid merged commit d73e1fc into Homebrew:master Sep 28, 2023
25 checks passed
@Bo98 Bo98 deleted the betterer-readall branch September 28, 2023 14:22
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 29, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants