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

Allow casks to be installed using the cask-source API #11859

Merged
merged 6 commits into from Aug 27, 2021

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Aug 14, 2021

  • 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?

This PR allows casks to be installed using the cask-source API when HOMEBREW_JSON_CORE is set.

Now that this applies to casks, I think it may make more sense for the environment variable to be named HOMEBREW_INSTALL_FROM_API (or similar). Thoughts? This should be a simple change (just replace all occurrences) so I'll do this in a separate PR.

In only somewhat major change is that casks now have Cask::Cask#source method that returns the source code of the cask (or nil if it wasn't set, although I don't know of a situation where this would happen because all casks loaded with Cask::CaskLoader should have it set).


The biggest issue I'm currently having is that checking whether a cask is outdated can be a bit problematic because casks can have different versions based on the operating system (e.g. onyx). Because the versions API is generated on a runner with macOS 10.15.7. This means that for casks like onyx, the version listed in /api/versions-casks-json is only the Catalina version. Therefore, Cask::Cask#outdated? return true for onyx on my Big Sur machine prompting me to upgrade the cask. Yet, when I run brew upgrade, it uses the source which defines a newer version (because it now knows I'm on Big Sur). This means I'm in a cycle where it installs the correct version of the cask, prompts me to upgrade, then upgrades to the exact same version.

Any ideas on how to move forward here? I'm wondering if we need to refine the versions API to include information for every (supported at a minimum) OS. This would also require some work to change how MacOS.version works (unless we actually want to run the versions API generator on every OS.

I wonder if in Homebrew/formulae.brew.sh we could override the MacOS.version method to manually override an check each OS option. Is that the best way to move forward here?

@BrewTestBot
Copy link
Member

Review period will end on 2021-08-17 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 14, 2021
@MikeMcQuaid
Copy link
Member

I wonder if in Homebrew/formulae.brew.sh we could override the MacOS.version method to manually override an check each OS option. Is that the best way to move forward here?

This makes sense to me 👍🏻. I think it would be good for brew info --json to be able to output all this information.

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.

Code looks great, nice work 👏🏻

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 17, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@Rylan12
Copy link
Member Author

Rylan12 commented Aug 18, 2021

I think it would be good for brew info --json to be able to output all this information.

I agree, but I'm a bit unsure how to convey this without making a backwards-incompatible change. Any thoughts?

@MikeMcQuaid
Copy link
Member

I agree, but I'm a bit unsure how to convey this without making a backwards-incompatible change. Any thoughts?

I think we still provide the current version based on the version it's generated on but have a new versions key or similar which has macOS version subkeys. Thoughts?

@Rylan12
Copy link
Member Author

Rylan12 commented Aug 19, 2021

Yeah, that makes sense. Good thinking. I also know that we may have a few formulae with a similar issue for their dependencies. I'll give some thought to handling those as well.

It might take me some time (I'm moving into school next week so there's a lot to do)

@MikeMcQuaid
Copy link
Member

It might take me some time (I'm moving into school next week so there's a lot to do)

No rush, good luck!

@Rylan12
Copy link
Member Author

Rylan12 commented Aug 27, 2021

Alright, the upgrade issue is now resolved so I'm going to merge this now so maintainers can start to play with it.

I think the next step is to rename HOMEBREW_JSON_CORE 🎉

@Rylan12 Rylan12 merged commit 8690d66 into Homebrew:master Aug 27, 2021
@Rylan12 Rylan12 deleted the cask-json branch August 27, 2021 03:58
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 27, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants