Cask loader improvements #14472

apainintheneck · 2023-02-01T03:47:46Z

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 includes two little improvements to the cask loader that are tangentially related to some of the cask upgrade problems that we've seen in the last few days. I don't think they will necessarily solve all of those problems but I did find some things that could be improved.

Sanity Check for Cask Token

Essentially, we hit the API here without any validation of the cask token. This means that when this is called with a value that is clearly wrong like a file path it decides to hit the API anyway which is unnecessary. The regex is based on the HOMEBREW_TAP_CASK_REGEX.

brew/Library/Homebrew/tap_constants.rb

Line 7 in 7a5f614

HOMEBREW_TAP_CASK_REGEX = %r{^([\w-]+)/([\w-]+)/([a-z0-9\-_]+)$}.freeze

The CaskSource.available? method is used in the CaskLoader when deciding whether or not to try and load the cask by downloading the cask source from the API.

brew/Library/Homebrew/cask/cask_loader.rb

Lines 373 to 375 in 7a5f614

    
           if Homebrew::EnvConfig.install_from_api? && !need_path && Homebrew::API::CaskSource.available?(ref) 
        
             return FromAPILoader.new(ref) 
        
           end

This should eliminate problems where an obviously invalid token is interpolated into then cask url path which is noticeable when using the debug flag.

Before

~ [1]$ brew install -n 'c/l/e/arly..invalid-token' --cask --debug
/usr/local/Homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/3.6.20-157-g1e4ae4e\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 10.15.7\)\ curl/7.64.1 --header Accept-Language:\ en --max-time 5 --retry 3 --fail https://formulae.brew.sh/api/cask-source/c/l/e/arly..invalid-token.rb

This url interpolation issue can be seen in #14463 for example. This shouldn't change behavior but will save a call to the API when we try to load a cask from a path.

Fix `FromAPILoader.can_load?` Logic

The previous logic didn't work as expected.

brew/Library/Homebrew/cask/cask_loader.rb

Lines 365 to 368 in 7a5f614

    
           if loader_class == FromTapLoader && Homebrew::EnvConfig.install_from_api? && 
        
              ref.start_with?("homebrew/cask/") && FromAPILoader.can_load?(ref) 
        
             return FromAPILoader.new(ref) 
        
           end

It's impossible for a cask reference to start with homebrew/cask/ and satisfy the requirements for FromAPILoader.can_load?

brew/Library/Homebrew/cask/cask_loader.rb

Lines 195 to 197 in 7a5f614

    
           def self.can_load?(ref) 
        
             Homebrew::API::Cask.all_casks.key? ref 
        
           end

brew/Library/Homebrew/api/cask.rb

Lines 19 to 28 in 7a5f614

    
           def all_casks 
        
             @all_casks ||= begin 
        
               json_casks = Homebrew::API.fetch_json_api_file "cask.json", 
        
                                                              target: HOMEBREW_CACHE_API/"cask.json" 
        
               json_casks.to_h do |json_cask| 
        
                 [json_cask["token"], json_cask.except("token")] 
        
               end 
        
             end 
        
           end

FromAPILoader.can_load? eventually ends up checking the cask reference against the keys in cask.json loaded as a hash which are the token attributes in the JSON. The tokens in the cask.json file are not prefixed with homebrew/cask/ so it's impossible to have that prefix and pass the .can_load? check.

[
  {"token":"0-ad","full_token":"0-ad","tap":"homebrew/cask","name":["0 A.D."],"desc":"Real-time strategy game","homepage":"https://play0ad.com/","url":"https://releases.wildfiregames.com/0ad-0.0.26-alpha-osx64.dmg","appcast":null,"version":"0.0.26-alpha","versions":{},"installed":null,"outdated":false,"sha256":"f8f0f9237d33f3b2acabc1d5b50ee6da32768231d5610a2ff52d1e65df76bf2c","artifacts":[{"app":["0 A.D..app"]},{"zap":[{"trash":"~/Library/Saved Application State/com.wildfiregames.0ad.savedState"}]}],"caveats":null,"depends_on":{"macos":{">=":["10.12"]}},"conflicts_with":null,"container":null,"auto_updates":null,"languages":[],"variations":{"arm64_ventura":{"url":"https://releases.wildfiregames.com/0ad-0.0.26-alpha-osx-aarch64.dmg","sha256":"3ef9a974ffa6f32577ba54f73b34a9d81a3798781fd8e30ea836626e3fdd3ac5"},"arm64_monterey":{"url":"https://releases.wildfiregames.com/0ad-0.0.26-alpha-osx-aarch64.dmg","sha256":"3ef9a974ffa6f32577ba54f73b34a9d81a3798781fd8e30ea836626e3fdd3ac5"},"arm64_big_sur":{"url":"https://releases.wildfiregames.com/0ad-0.0.26-alpha-osx-aarch64.dmg","sha256":"3ef9a974ffa6f32577ba54f73b34a9d81a3798781fd8e30ea836626e3fdd3ac5"}}},
  {"token":"010-editor","full_token":"010-editor","tap":"homebrew/cask","name":["010 Editor"],"desc":"Text editor","homepage":"https://www.sweetscape.com/","url":"https://download.sweetscape.com/010EditorMac64Installer13.0.1.dmg","appcast":null,"version":"13.0.1","versions":{},"installed":null,"outdated":false,"sha256":"8cf8c6b38a0d08b4308ebdc4cc7fabb4884c41b321a92becc4be0b87e7c3fafe","artifacts":[{"app":["010 Editor.app"]}],"caveats":null,"depends_on":{},"conflicts_with":null,"container":null,"auto_updates":null,"languages":[],"variations":{}},
  ...
]

So if that bit of code cannot possibly be called right now, where are the casks getting loaded from when HOMEBREW_INSTALL_FROM_API is set? There is a backup call to the API that is doing all the heavy lifting.

brew/Library/Homebrew/cask/cask_loader.rb

Lines 373 to 375 in 7a5f614

    
           if Homebrew::EnvConfig.install_from_api? && !need_path && Homebrew::API::CaskSource.available?(ref) 
        
             return FromAPILoader.new(ref) 
        
           end

This changes the logic to first validate that the token seems plausible, second delete the prefix "homebrew/cask/" if it's there and then third check if the token exists in the JSON. It also moves that into FromAPILoader.can_load? to try and clean up CaskLoader.for a little bit.

Before

~ $ brew install -n homebrew/cask/1password --cask --debug
/usr/local/Homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/3.6.20-157-g1e4ae4e\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 10.15.7\)\ curl/7.64.1 --header Accept-Language:\ en --fail --progress-bar --max-time 5 --retry 3 --location --remote-time --output /Users/kevinrobell/Library/Caches/Homebrew/api/cask.json --time-cond /Users/kevinrobell/Library/Caches/Homebrew/api/cask.json --compressed --silent https://formulae.brew.sh/api/cask.json
Warning: Cask 'homebrew/cask/1password' is unavailable.
Please tap it and then try again: brew tap homebrew/cask

After

~ $ brew install -n homebrew/cask/1password --cask --debug
/usr/local/Homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/3.6.20-148-gaa35cc5\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 10.15.7\)\ curl/7.64.1 --header Accept-Language:\ en --fail --progress-bar --max-time 5 --retry 3 --location --remote-time --output /Users/kevinrobell/Library/Caches/Homebrew/api/cask.json --time-cond /Users/kevinrobell/Library/Caches/Homebrew/api/cask.json --compressed --silent https://formulae.brew.sh/api/cask.json
==> Would install 1 cask:
1password

This allows homebrew/cask/caskname to work with the FromAPILoader. Also, creates new constant to hold the regex to validate main tap casks.

BrewTestBot · 2023-02-01T03:48:02Z

Review period will end on 2023-02-02 at 03:47:46 UTC.

apainintheneck · 2023-02-01T03:50:29Z

Library/Homebrew/tap_constants.rb

+# Match main cask taps' casks, e.g. `homebrew/cask/somecask` or `somecask`
+HOMEBREW_MAIN_TAP_CASK_REGEX = %r{^(homebrew/cask/)?[a-z0-9\-_]+$}.freeze


Not sure if this is the best place to put this but I couldn't find anywhere else that made more sense.

apainintheneck · 2023-02-01T03:51:23Z

Library/Homebrew/cask/cask_loader.rb

@@ -356,18 +361,12 @@ def self.for(ref, need_path: false)
        FromInstanceLoader,
        FromContentLoader,
        FromURILoader,
+        FromAPILoader,


Run before FromTapLoader to preserve the same intended behavior as before.

Currently, if the main cask tap is specified but the cask doesn't exist from the API it will skip the FromAPILoader and will try the FromTapLoader which will throw an error.

~ [1]$ brew install -n homebrew/cask/1passwordless --cask --debug /usr/local/Homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/3.6.20-148-gaa35cc5-dirty\ $Macintosh\;\ Intel\ Mac\ OS\ X\ 10.15.7$\ curl/7.64.1 --header Accept-Language:\ en --fail --progress-bar --max-time 5 --retry 3 --location --remote-time --output /Users/kevinrobell/Library/Caches/Homebrew/api/cask.json --time-cond /Users/kevinrobell/Library/Caches/Homebrew/api/cask.json --compressed --silent https://formulae.brew.sh/api/cask.json Warning: Cask 'homebrew/cask/1passwordless' is unavailable. Please tap it and then try again: brew tap homebrew/cask

Maybe it'd be better to just try to load all cask references that start with the homebrew/cask/ in the FromAPILoader so that we can throw an error that doesn't suggest tapping the main cask tap (the same casks should be available from both places, right?). This is the current behavior though so I didn't want to change it without some discussion.

Maybe it'd be better to just try to load all cask references that start with the homebrew/cask/ in the FromAPILoader so that we can throw an error that doesn't suggest tapping the main cask tap (the same casks should be available from both places, right?).

This makes sense to me (but can wait until another PR if easier).

In that case, this can be simplified even further.

Not sure I'm brave enough to make that change in this PR. 😁

BrewTestBot · 2023-02-01T19:13:04Z

Review period ended.

MikeMcQuaid

Great work here! Happy to merge as-is.

MikeMcQuaid · 2023-02-01T19:13:48Z

Library/Homebrew/cask/cask_loader.rb

@@ -356,18 +361,12 @@ def self.for(ref, need_path: false)
        FromInstanceLoader,
        FromContentLoader,
        FromURILoader,
+        FromAPILoader,


Maybe it'd be better to just try to load all cask references that start with the homebrew/cask/ in the FromAPILoader so that we can throw an error that doesn't suggest tapping the main cask tap (the same casks should be available from both places, right?).

This makes sense to me (but can wait until another PR if easier).

apainintheneck · 2023-02-02T02:21:25Z

I'm wondering if the second check for the FromAPILoader is even necessary after this change.

brew/Library/Homebrew/cask/cask_loader.rb

Lines 373 to 375 in 7a5f614

    
           if Homebrew::EnvConfig.install_from_api? && !need_path && Homebrew::API::CaskSource.available?(ref) 
        
             return FromAPILoader.new(ref) 
        
           end

This will only get triggered if the cask was not in the cask.json we got from the API (or it would have been loaded with the FromAPILoader the first time) but does exist in the homebrew/cask repo which sounds impossible.

Edit: It seems to get regenerated every hour so there is the possibility that a cask exists but isn't in the cask.json file. In other words, this makes sense to me.

apainintheneck · 2023-02-02T08:10:30Z

I'll merge this in tomorrow.

After 32a0877 this logic has been changed so it's now always covered by `FromAPILoader.can_load?`.

MikeMcQuaid · 2023-02-03T08:37:12Z

Thanks again @apainintheneck!

Rylan12

Thanks, this looks great, @apainintheneck!

apainintheneck added 2 commits January 30, 2023 23:52

Add sanity check for cask token

4917a35

Load cask from API with fullname

e0ad960

This allows homebrew/cask/caskname to work with the FromAPILoader. Also, creates new constant to hold the regex to validate main tap casks.

apainintheneck added the cask Homebrew Cask label Feb 1, 2023

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 1, 2023

apainintheneck commented Feb 1, 2023

View reviewed changes

apainintheneck marked this pull request as ready for review February 1, 2023 06:13

apainintheneck added the install from api Relates to API installs label Feb 1, 2023

MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Feb 1, 2023

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 1, 2023

BrewTestBot approved these changes Feb 1, 2023

View reviewed changes

MikeMcQuaid approved these changes Feb 1, 2023

View reviewed changes

apainintheneck added 2 commits February 2, 2023 18:40

Handle merge conflict

2d93cbe

Remove redundant FromAPILoader attempt

15c1930

After 32a0877 this logic has been changed so it's now always covered by `FromAPILoader.can_load?`.

MikeMcQuaid merged commit 820ec54 into Homebrew:master Feb 3, 2023

Rylan12 reviewed Feb 3, 2023

View reviewed changes

apainintheneck deleted the cask-loader-improvements branch February 11, 2023 19:08

github-actions bot added the outdated PR was locked due to age label Mar 14, 2023

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cask loader improvements #14472

Cask loader improvements #14472

apainintheneck commented Feb 1, 2023

BrewTestBot commented Feb 1, 2023

apainintheneck Feb 1, 2023

apainintheneck Feb 1, 2023

apainintheneck Feb 1, 2023

MikeMcQuaid Feb 1, 2023

apainintheneck Feb 2, 2023

apainintheneck Feb 2, 2023

BrewTestBot commented Feb 1, 2023 •

edited

MikeMcQuaid left a comment

MikeMcQuaid Feb 1, 2023

apainintheneck commented Feb 2, 2023 •

edited

apainintheneck commented Feb 2, 2023

MikeMcQuaid commented Feb 3, 2023

Rylan12 left a comment

	if Homebrew::EnvConfig.install_from_api? && !need_path && Homebrew::API::CaskSource.available?(ref)
	return FromAPILoader.new(ref)
	end

	if loader_class == FromTapLoader && Homebrew::EnvConfig.install_from_api? &&
	ref.start_with?("homebrew/cask/") && FromAPILoader.can_load?(ref)
	return FromAPILoader.new(ref)
	end

	def self.can_load?(ref)
	Homebrew::API::Cask.all_casks.key? ref
	end

	def all_casks
	@all_casks \|\|= begin
	json_casks = Homebrew::API.fetch_json_api_file "cask.json",
	target: HOMEBREW_CACHE_API/"cask.json"

	json_casks.to_h do \|json_cask\|
	[json_cask["token"], json_cask.except("token")]
	end
	end
	end

		# Match main cask taps' casks, e.g. `homebrew/cask/somecask` or `somecask`
		HOMEBREW_MAIN_TAP_CASK_REGEX = %r{^(homebrew/cask/)?[a-z0-9\-_]+$}.freeze

Cask loader improvements #14472

Cask loader improvements #14472

Conversation

apainintheneck commented Feb 1, 2023

Sanity Check for Cask Token

Before

Fix FromAPILoader.can_load? Logic

Before

After

BrewTestBot commented Feb 1, 2023

apainintheneck Feb 1, 2023

Choose a reason for hiding this comment

apainintheneck Feb 1, 2023

Choose a reason for hiding this comment

apainintheneck Feb 1, 2023

Choose a reason for hiding this comment

MikeMcQuaid Feb 1, 2023

Choose a reason for hiding this comment

apainintheneck Feb 2, 2023

Choose a reason for hiding this comment

apainintheneck Feb 2, 2023

Choose a reason for hiding this comment

BrewTestBot commented Feb 1, 2023 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Feb 1, 2023

Choose a reason for hiding this comment

apainintheneck commented Feb 2, 2023 • edited

apainintheneck commented Feb 2, 2023

MikeMcQuaid commented Feb 3, 2023

Rylan12 left a comment

Choose a reason for hiding this comment

Fix `FromAPILoader.can_load?` Logic

BrewTestBot commented Feb 1, 2023 •

edited

apainintheneck commented Feb 2, 2023 •

edited