Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: only block cask install on Linux #18808

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

SMillerDev
Copy link
Member

@SMillerDev SMillerDev commented Nov 23, 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?

This is the first step to allowing font (and later binary) casks to be installable on Linux.
TODO:

  • Think about how to handle brew search --cask X
  • Think about how to handle brew info --cask X

@SMillerDev SMillerDev force-pushed the feat/cask/block_install branch 4 times, most recently from 733c151 to 22d6b09 Compare November 24, 2024 10:52
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.

Makes sense so far! Probably want to make --cask explicitly required on Linux until this is (much) further along?

@SMillerDev
Copy link
Member Author

Probably want to make --cask explicitly required on Linux until this is (much) further along?

You mean for install? I'll have to check, but I think you have to manually tap the cask repo for this to do anything on Linux for now. That seems like a sufficient barrier.

@MikeMcQuaid
Copy link
Member

You mean for install? I'll have to check, but I think you have to manually tap the cask repo for this to do anything on Linux for now. That seems like a sufficient barrier.

@SMillerDev Ok, as long as brew install docker or brew install google-chrome on Linux retains the same behaviour as before 👍🏻

@SMillerDev
Copy link
Member Author

I'll do an extra check for those specific ones.

What do you think should happen to search and info? Just allow them for casks?

@MikeMcQuaid
Copy link
Member

What do you think should happen to search and info? Just allow them for casks?

I think probably require --cask or something?

@SMillerDev
Copy link
Member Author

Tested this in a codespace, it has the same result, but a different output.

Before this PR:

linuxbrew@codespaces-069e0c:~/.linuxbrew/Homebrew$ brew install google-chrome
==> Downloading https://formulae.brew.sh/api/formula.jws.json

Warning: No available formula with the name "google-chrome". Did you mean google-benchmark?
==> Searching for similarly named formulae...
==> Formulae
google-benchmark

To install google-benchmark, run:
  brew install google-benchmark

After this PR:

linuxbrew@codespaces-069e0c:~/.linuxbrew/Homebrew$ brew install google-chrome
==> Downloading https://formulae.brew.sh/api/cask.jws.json
##O=#    #                                                                                                                                         
Error: macOS is required for this software.

@MikeMcQuaid
Copy link
Member

@SMillerDev What about brew install --cask google-chrome

I think the prior behaviour is better on Linux for now, sorry, but I think if you added --cask then the latter would make sense.

@SMillerDev
Copy link
Member Author

I'll reinstate that. Seems like I was too eager with the CLI changes. After that and fixing the tests, are you fine to merge this while I work on the font installing? Or you'd rather bundle it and merge in one go?

@MikeMcQuaid
Copy link
Member

@SMillerDev I'm game to merge smaller/incremental changes here, yeh 👍🏻

@SMillerDev SMillerDev force-pushed the feat/cask/block_install branch from 22d6b09 to b0df86e Compare December 2, 2024 18:29
@SMillerDev
Copy link
Member Author

Okay, with this latest change that behaviour should be reinstated.

@SMillerDev SMillerDev marked this pull request as ready for review December 2, 2024 18:29
@SMillerDev SMillerDev force-pushed the feat/cask/block_install branch 4 times, most recently from 3b3a7bb to 526d311 Compare December 4, 2024 21:58
@SMillerDev SMillerDev force-pushed the feat/cask/block_install branch 6 times, most recently from 9e5bdfe to 1d907ca Compare December 14, 2024 14:45
@SMillerDev SMillerDev force-pushed the feat/cask/block_install branch from 1d907ca to cb23433 Compare December 14, 2024 14:49
@SMillerDev
Copy link
Member Author

Looks like all checks are green and the behavior was restored as requested.
@MikeMcQuaid do you mind taking another look?

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.

Looks good, thanks @SMillerDev!

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Dec 16, 2024
Merged via the queue into master with commit 640f215 Dec 16, 2024
28 checks passed
@MikeMcQuaid MikeMcQuaid deleted the feat/cask/block_install branch December 16, 2024 12:27
@apainintheneck
Copy link
Contributor

Great work here!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants