Skip to content

Comments

bump: fix arch-specific version output#21596

Draft
samford wants to merge 6 commits intomainfrom
bump-fix-arch-specific-version-output
Draft

bump: fix arch-specific version output#21596
samford wants to merge 6 commits intomainfrom
bump-fix-arch-specific-version-output

Conversation

@samford
Copy link
Member

@samford samford commented Feb 19, 2026

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

  • AI was used to generate or assist with generating this PR. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes.

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 files 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 also 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.


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. This effectively ensures that the current and new versions are only split in the output when the version differs.
  • The deprecated[:general] value wasn't being properly set 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_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).


Lastly, this cleans up version_info value usage in retrieve_and_display_info_and_open_pr, making the code a little more readable in places.

Copilot AI review requested due to automatic review settings February 19, 2026 21:20
Copy link
Contributor

Copilot AI left a 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 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.general when arm/intel values are absent.
  • Refactor version_info usage in retrieve_and_display_info_and_open_pr for readability.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

This is great! Thanks @samford!

`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 files 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. This effectively ensures that the
current and new versions are only split in the output when the version
differs.
* The `deprecated[:general]` value wasn't being properly set 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 properly 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).
@samford samford force-pushed the bump-fix-arch-specific-version-output branch from fac6208 to 3a17a64 Compare February 21, 2026 03:08
@samford
Copy link
Member Author

samford commented Feb 21, 2026

The latest push addresses the issue flagged by Copilot where the newer_than_upstream values can be incorrect if there are multiple current versions or multiple new versions but not both. I've manually tested this and it seems to work as expected. I think I've accounted for the cases that we can encounter in practice but I won't make any guarantees as this is a challenge to reason about and there may be some scenario that I haven't considered.

Past this, the next bump-related PR that I have coming is to introduce some basic handling of single-arch version bumps, which will address another group of issues that we're seeing in autobump for casks (i.e., bump trying to update to a "skipped" version). I've had that implemented for a few months but only recently found a workaround for a blocker to allow it to move forward. There's some overlap in the code changes, so I'll create a PR for it after this is merged.

Edit: I'm working on a better way of handling the newer_than_upstream logic (i.e., without hardcoding comparison pairs), so I've temporarily set this as a draft while I do some refactoring.

@samford samford marked this pull request as draft February 22, 2026 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants