-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
formula_auditor: audit revision and compatibility_version
#20936
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
formula_auditor: audit revision and compatibility_version
#20936
Conversation
d7bae0f to
21b5d67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for auditing compatibility_version changes in formulae and extends the existing revision audit to validate dependency relationships.
- Introduces a new
audit_compatibility_versionmethod that ensures compatibility versions don't decrease and only increment by 1, and that dependent formulae bump their revisions accordingly - Extends
audit_revisionto validate that when a formula's revision increases, changed dependencies must have their compatibility_version incremented by 1 - Refactors version info caching and adds a
changed_formulae_pathshelper method to identify changed formulae in a tap
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Library/Homebrew/formula_auditor.rb | Adds audit_compatibility_version method, extends audit_revision with dependency validation, adds changed_formulae_paths helper, and refactors version info caching to support multiple formulae |
| Library/Homebrew/test/formula_auditor_spec.rb | Adds comprehensive test coverage for both compatibility_version audits and revision-dependency relationship validation, including new test helper methods for formula creation and stubbing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rylan12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach looks good! I do think there's a few places where the wrong output from committed_version_info is being used, though.
It's possible that I'm misunderstanding, though, so feel free to correct me if I'm wrong here
4c7716e to
5c0782d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5c0782d to
99ccfd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
99ccfd7 to
3871cc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rylan12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I love the naming! Thanks for sticking through my nits 😅
One final set of comments, I still think there's a small edge case that's not handled, but I think it's unlikely (and potentially impossible) to get to this situation, so feel free to ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
200c827 to
2e994a9
Compare
Rylan12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I caught one more small thing, but good to merge without re-reviews
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2cd8da4 to
338f8dd
Compare
338f8dd to
0e57103
Compare
Add a new audit method to check if the `revision` and `compatibility_version` are consistent with each other. This ensures that when a formula's `compatibility_version` is increased, the `revision` of dependent formulas was also increased. Inversely, when a formula's `revision` is increased in the same PR as one of its dependencies, the `compatibility_version` of dependent formulae must be increased by 1. While we're here, DRY things up and improve some naming to me more readable/obvious. Co-authored-by: Rylan Polster <[email protected]>
0e57103 to
1cecd7a
Compare
I just ran into this new audit in osrf/homebrew-simulation#3272 (comment), which bumps the revision for two formulae to rebuild against the new libwebsockets upgraded in Homebrew/homebrew-core#256449, and it recommends that one of the formula needs a For a formula with multiple levels of recursive dependents that require revision bumps for each minor upgrade (such as the protobuf upgrade in Homebrew/homebrew-core#254203), would this also require |
|
@scpeters You are bottling in your tap, yes? It occurred to me if you weren't then this check is somewhat redundant and should perhaps be skipped. Would accept a PR for that.
In a case like that PR, only @p-linnane remind me: we now track this sort of thing better now, right? I remember historically we treated direct and indirect dependencies similarly for linkage tracking and now we don't, right? |
|
Earlier this year we started warning about indirect dependencies with linkage in an effort to fix instances of that. It's not a hard error IIRC. Up next on my list is to start warning about instances of |
|
@p-linnane thanks! Yeh, I think it might be good to start making it a hard error in homebrew/core at least 🤔 |
We are bottling in the osrf/simulation tap, but I'll open a PR to disable the audit that fails in osrf/homebrew-simulation#3272 and we can continue discussion there. |
I opened #21184 |
Add a new audit method to check if the
revisionandcompatibility_versionare consistent with each other.This ensures that when a formula's
compatibility_versionis increased, therevisionof dependent formulas was also increased.Inversely, when a formula's
revisionis increased in the same PR as one of its dependencies, thecompatibility_versionof dependent formulae must be increased by 1.While we're here, DRY things up and improve some naming to be more readable/obvious.