formula_auditor: audit revision and compatibility_version
#20936
Conversation
MikeMcQuaid
force-pushed
the
audit_revision_and_compatibility_version_relationships
branch
from
d7bae0f to
21b5d67
Compare
There was a problem hiding this comment.
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.
![github-code-quality[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
MikeMcQuaid
force-pushed
the
audit_revision_and_compatibility_version_relationships
branch
from
4c7716e to
5c0782d
Compare
There was a problem hiding this comment.
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.
MikeMcQuaid
force-pushed
the
audit_revision_and_compatibility_version_relationships
branch
from
5c0782d to
99ccfd7
Compare
There was a problem hiding this comment.
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.
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 <rslpolster@gmail.com>
MikeMcQuaid
force-pushed
the
audit_revision_and_compatibility_version_relationships
branch
from
99ccfd7 to
3871cc0
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| previous_version_info, origin_head_version_info = committed_version_info | ||
| return if origin_head_version_info.empty? | ||
|
|
||
| previous_compatibility_version = previous_version_info[:compatibility_version] || 0 |
There was a problem hiding this comment.
| previous_compatibility_version = previous_version_info[:compatibility_version] || 0 | |
| previous_compatibility_version = origin_head_version_info[:compatibility_version] || 0 |
Because committed_version_info doesn't stop iterating when compatibility_version changes, I think we need to use origin_head_version_info here to protect against cases like this:
| version | revision | compatibility_version | |
|---|---|---|---|
| current | 1.0.0 |
1 |
0 |
origin/HEAD |
1.0.0 |
1 |
1 |
| previous | 1.0.0 |
0 |
0 |
Right now, the compatibility_version should not decrease problem won't trigger because current_compatibility_version == previous_compatibility_version == 0, and also compatibility_increment == 0.
| current_version_scheme = formula.version_scheme | ||
|
|
||
| previous_committed, = committed_version_info | ||
| previous_version_info, = committed_version_info |
There was a problem hiding this comment.
| previous_version_info, = committed_version_info | |
| _, origin_head_version_info = committed_version_info |
Same thing here, this allows the following:
version |
revision |
version_scheme |
|
|---|---|---|---|
| current | 1.0.0 |
1 |
0 |
origin/HEAD |
1.0.0 |
1 |
1 |
| previous | 1.0.0 |
0 |
0 |
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.