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

cask: skip variations for inapplicable versions #17386

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

EricFromCanada
Copy link
Member

@EricFromCanada EricFromCanada commented May 29, 2024

  • 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 style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

When running to_hash_with_variations on a cask, its min/max macOS version requirements aren’t considered which may result in:

  • extra entries in its list of variations for macOS versions that aren’t actually supported, if the oldest on_system block uses :or_older - anki, hammerspoon
  • same as above, but the unsupported macOS versions have the latest version number listed, caused by the on_system block not using :or_older, which results in null values being inserted into the variations list - apparency, omnidisksweeper, saoimageds9, sf-symbols
  • same as above, but every item in the variations list also has the same depends_on key if specified as a list - calhash

The first two are solved by checking if each bottle tag is included in the cask’s range of macOS requirements, which itself required adding MacOSRequirement.allows? to check if a macOS requirement allows a given macOS version.

The last occurs because each call in cask.rb of public_send(hash_method).each do |key, value| gets a different result for depends_on, because the stock inspect method of the MacOSVersion class includes a random hex value, resulting in it being included in the hash every time. Customizing the inspect method to just print the class’s version value avoids this.

This change also highlights complications like how some casks' on_system blocks don't line up with their macOS version requirements, e.g.:

  • depends_on being inside on_system blocks - airfoil
  • depends_on excluding entire on_system blocks - appcleaner

There are probably others, but these can be manually resolved in time.

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.

Looking good so far! Will wait for @dduugg to chime in here!

Library/Homebrew/macos_version.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/cask.rb Outdated Show resolved Hide resolved
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.

This makes more sense to me, thanks @EricFromCanada!

Good to 🚢 if @carlocab is 👍🏻

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

💯

@MikeMcQuaid MikeMcQuaid merged commit 93e58fb into master Jun 4, 2024
25 checks passed
@MikeMcQuaid MikeMcQuaid deleted the cask-null-variations branch June 4, 2024 09:04
@Bo98
Copy link
Member

Bo98 commented Jun 20, 2024

This seems to have broken variation support: https://github.com/orgs/Homebrew/discussions/5448. Likely because depends_on.macos can't necessarily be trusted if it is inside an on_system block itself.

Are you able to take a look or would you prefer a revert?

@EricFromCanada
Copy link
Member Author

Are you able to take a look or would you prefer a revert?

I will take a look.

@EricFromCanada
Copy link
Member Author

EricFromCanada commented Jun 21, 2024

Likely because depends_on.macos can't necessarily be trusted if it is inside an on_system block itself.

Looks like this is the case. When run on Monterey or older:

$ HOMEBREW_NO_INSTALL_FROM_API=1 brew info --cask macupdater | head -n 1
==> macupdater: 2.3.15 (auto_updates)
$ HOMEBREW_NO_INSTALL_FROM_API= brew info --cask macupdater | head -n 1
==> macupdater: 3.3.1 (auto_updates)

All the currently-affected casks are fixed by these two commits in Homebrew/homebrew-cask#176710, but that's on hold while I look into how to a) update cask/audit.rb to not complain about depends_on in a multi-version-block cask being less than the minimum OS for the newest version, and b) writing a check to prevent block-specific depends_on from being added.

For now, removing this line will fix the noted behaviour for the affected casks.

Both the line mentioned above and the audit issues before that are affected by the inability to determine if code is being run inside an on_os block or not. All I've found so far is checking if they exist with on_system_blocks_exist?.

@Bo98
Copy link
Member

Bo98 commented Jun 21, 2024

Both the line mentioned above and the audit issues before that are affected by the inability to determine if code is being run inside an on_os block or not. All I've found so far is checking if they exist with on_system_blocks_exist?.

You could modify the depends_on DSL to check @called_in_on_system_block and store it somewhere, somewhat like how set_unique_stanza sets #{stanza}_set_in_block albeit for a different purpose.

We should fix the audit indeed, though backwards compatibility with the previous way of handling depends_on is still probably desirable given brew info --json --variations is public API.

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.

None yet

5 participants