bump: fix arch-specific version output#21596
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes brew bump’s version reporting when casks have arch-specific blocks but a single consolidated version, and improves how “(deprecated)” annotations are displayed for arch-specific output by capturing deprecation state under simulated architectures.
Changes:
- Capture per-arch deprecation status during arch simulation and use it in output formatting.
- Make arch-specific “current version” output fall back to
current_version.generalwhenarm/intelvalues are absent. - Refactor
version_infousage inretrieve_and_display_info_and_open_prfor readability.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fac6208 to
3a17a64
Compare
|
The latest push addresses the issue flagged by Copilot where the Past this, the next Edit: I'm working on a better way of handling the |
3a17a64 to
2de99b7
Compare
2de99b7 to
9a20c07
Compare
|
The latest push refactors the I've also introduced a different way of checking if a I think this is all that I intend to accomplish in this PR, so this is ready for review now. |
`brew bump` uses one `general` version in the `BumpVersionParser`
object when the ARM and Intel versions are identical. However, only
the `arm` and `intel` values are used when printing the current
versions for each arch, so bump won't successfully print the current
versions for casks that have one `version` but different assets for
each arch:
```
Current cask version: arm:
intel: (deprecated)
```
This addresses the issue by modifying the related output strings to
fall back to `current_version.general` if the `arm` and `intel` values
are blank.
`brew bump` adds a " (deprecated)" annotation after the current
version text if the formula/cask is deprecated and this works when
printing one version but it's confusing when arch-specific versions
are printed because it can incorrectly suggest that only the Intel
version is deprecated even if both ARM and Intel share the same
top-level deprecation. For example, a cask with an implicit
deprecation from a `disable!` call with a future date appears like
this:
```
Current cask version: arm: 1.2.3
intel: 1.2.3 (deprecated)
```
This addresses the issue by modifying how the deprecated annotation
is added after version text, so that it will correctly appear after
the arch-specific version(s) it applies to. Additionally,
`formula_or_cask.deprecated?` depends on the execution environment, so
this captures the value in `retrieve_versions_by_arch` when the arch
is simulated. This setup ensures that `bump` is able to correctly
handle packages where one arch is deprecated but another isn't.
We already create variables for `version_info` values that are reused throughout bump's `retrieve_and_display_info_and_open_pr` method, so this adds additional variables for `multiple_versions` and `newer_than_upstream` to tidy up related usage.
The `multiple_versions` boolean is true if the current or new versions are split based on arch but this can cause unexpected behavior when `multiple_versions` is used as a conditional. In some places we need to check whether there are multiple current versions and in other places we need to check if there are multiple new versions, so the existing value isn't granular enough. This may not be a problem when both current and new have the same version setup but it can cause issues when there's a difference. This changes `multiple_versions` to a hash with boolean values and updates related conditions to use the contextually-appropriate value. This resolves a couple of issues: * The current version for multi-arch casks with one `version` were being split into arm/intel values in the output when the new versions differ based on arch and vice versa. These changes ensure that the current and new versions are only split in the output when the version differs. * The `deprecated[:general]` value wasn't being set properly when the current and new versions didn't have the same version setup, as the fallback value specifically needs to be set when there aren't multiple current versions. Besides that, we also need to specifically check if there are multiple current versions to be able to correctly identify current versions that are newer than the upstream version when both current and new don't have the same version setup but this will be addressed in another commit.
The `newer_than_upstream` booleans can be incorrect when there's one current version but multiple new versions or vice versa. This occurs because the related comparisons are strictly done between the same `BumpVersionParser` values and it doesn't work as expected in this scenario (i.e., the `:arm`/`:intel` values are `nil` when `:general` is used for the version and vice versa). This adds additional logic to define which comparisons are used depending on whether the current and/or new versions have multiple versions, notably comparing against the `:general` version when there's a difference between current and new. One caveat is that `bump` will fall back to the `newer_than_upstream[:arm]` value when the current version uses one version and `:general` isn't present (as is the case when `multiple_versions[:new]` is true) but this aligns with other usage of ARM as the favored arch in `bump` and `bump-cask-pr`. This isn't the most elegant solution overall but it works as expected (I wasn't able to achieve the same result through more modest modifications, not to say that it's not possible).
This refactors the `newer_than_upstream` logic to make it a little more flexible, using a programmatic approach to create the version comparison pairs rather than hardcoded arch values. This also extracts the logic for defining `multiple_versions` and `newer_than_upstream` into a separate `compare_versions` method. Besides simplifying `retrieve_versions_by_arch`, this allowed me to add a reasonable test for this code. It's worth noting that this compares the current version to the highest new version when the current version does not differ by arch but the new version does. There isn't one clear way to handle this (e.g., you could treat the current version as newer than upstream if it's higher than any new version) but this felt like the least confusing behavior. I'm also working on modifying `bump` to handle single-arch version bumps, so comparing to the highest new version also makes more sense in that context (i.e., if the current version is treated as newer when it's higher than any new version, a lower new version for Intel could prevent `bump` from updating to a higher new version for ARM). For what it's worth, this removes the `|| newer_than_upstream[:arm]` fallback for when `multiple_versions[:current]` isn't true in `retrieve_and_display_info_and_open_pr`, as the `:general` value should be present in that situation (based on how this is handled here).
We use "current" terminology in livecheck to refer to the current version(s) that a formula/cask uses and I think this is easy to understand. This renames the `old_versions` variable to `current_versions`, which better reflects the intention and aligns with other related variable names throughout `bump`.
9a20c07 to
c11ecb8
Compare
brew lgtm(style, typechecking and tests) with your changes locally?brew bumpuses onegeneralversion in theBumpVersionParserobject when the ARM and Intel versions are identical. However, only thearmandintelvalues are used when printing the current versions for each arch, so bump won't successfully print the current versions for casks that have oneversionbut different assets for each arch:This addresses the issue by modifying the related output strings to fall back to
current_version.generalif thearmandintelvalues are blank.brew bumpalso adds a " (deprecated)" annotation after the current version text if the formula/cask is deprecated and this works when printing one version but it's confusing when arch-specific versions are printed because it can incorrectly suggest that only the Intel version is deprecated even if both ARM and Intel share the same top-level deprecation. For example, a cask with an implicit deprecation from adisable!call with a future date appears like this:This addresses the issue by modifying how the deprecated annotation is added after version text, so that it will correctly appear after the arch-specific version(s) it applies to. Additionally,
formula_or_cask.deprecated?depends on the execution environment, so this captures the value inretrieve_versions_by_archwhen the arch is simulated. This setup ensures thatbumpis able to correctly handle packages where one arch is deprecated but another isn't.The
multiple_versionsboolean is true if the current or new versions are split based on arch but this can cause unexpected behavior whenmultiple_versionsis used as a conditional. In some places we need to check whether there are multiple current versions and in other places we need to check if there are multiple new versions, so the existing value isn't granular enough. This may not be a problem when both current and new have the same version setup but it can cause issues when there's a difference.This changes
multiple_versionsto a hash with boolean values and updates related conditions to use the contextually-appropriate value. This resolves a couple of issues:versionwere being split into arm/intel values in the output when the new versions differ based on arch and vice versa. These changes ensure that the current and new versions are only split in the output when the version differs.deprecated[:general]value wasn't being set properly when the current and new versions didn't have the same version setup, as the fallback value specifically needs to be set when there aren't multiple current versions.The
newer_than_upstreambooleans can be incorrect when there's one current version but multiple new versions or vice versa. This occurs because the related comparisons are strictly done between the sameBumpVersionParservalues and it doesn't work as expected in this scenario (i.e., the:arm/:intelvalues arenilwhen:generalis used for the version and vice versa).This adds additional logic to define which comparisons are used depending on whether the current and/or new versions have multiple versions, notably comparing against the
:generalversion when there's a difference between current and new. This also extracts the logic for definingmultiple_versionsandnewer_than_upstreaminto a separatecompare_versionsmethod. Besides simplifyingretrieve_versions_by_arch, this allowed me to add a reasonable test for this code.It's worth noting that this compares the current version to the highest new version when the current version does not differ by arch but the new version does. There isn't one clear way to handle this (e.g., you could treat the current version as newer than upstream if it's higher than any new version) but this felt like the least confusing behavior. I'm also working on modifying
bumpto handle single-arch version bumps, so comparing to the highest new version also makes more sense in that context (i.e., if the current version is treated as newer when it's higher than any new version, a lower new version for Intel could preventbumpfrom updating to a higher new version for ARM).Lastly, this cleans up
version_infovalue usage inretrieve_and_display_info_and_open_pr, making the code a little more readable in places, and renamesold_versionstocurrent_versions(for clarity).