Delay loading from cask source api #14820

apainintheneck · 2023-02-26T19:37:22Z

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?

The idea here is to delay the loading of casks from the source API until the last possible moment. That means either right before installation (during the fetch phase to be precise) or during the uninstallation process if we can't load the original cask file.

As a result of this change I had to modify how info was loaded into the cask object from the API. The git tap head and languages info needed to be passed in as well. I tried reorganizing it by adding the Cask#populate_from_api! method but if you all think it's better to just pass those as params to the constructor I'm fine with that too. I also load the *flight artifacts with empty blocks. This info along with the languages is used to decided whether a cask needs the cask file during installs and uninstalls.

There are also a few changes to the cask installer. The big one is that we now try to uninstall using the installed cask file if at all possible with the brew uninstall and brew uninstall --zap commands. Previously we would just try to uninstall using the most recent cask info. I added a few tests for these changes too. This is necessary to make sure we have the cask file if needed during uninstalls.

~~Marked as draft because I want to do a bit more local testing.~~ 👍

To test you can try installing and uninstalling casks with language or *flight stanzas.

For casks with certain stanzas, *flight and language stanzas in this case, we need to use the caskfile to install them correctly. The JSON API is not an option. This delays loading from the source API until just before we try to install one of these casks. This reduces the number of requests we make to the source API.

BrewTestBot · 2023-02-26T19:37:34Z

Review period will end on 2023-02-28 at 00:00:00 UTC.

apainintheneck · 2023-02-27T01:10:54Z

Library/Homebrew/cask/cask_loader.rb

@@ -319,15 +299,21 @@ def load(config:)
            # convert generic string replacements into actual ones
            artifact = cask.loader.from_h_hash_gsubs(artifact, appdir)
            key = artifact.keys.first
-            send(key, *artifact[key])
+            if artifact[key].nil?


Maybe it'd be better to be explicit here but at the moment the only artifacts with null values are *flight blocks.

apainintheneck · 2023-02-27T01:11:20Z

Library/Homebrew/cask/dsl.rb

@@ -77,7 +77,6 @@ class DSL
      :depends_on,
      :homepage,
      :language,
-      :languages,


This now gets defined explicitly in the cask class.

apainintheneck · 2023-02-27T01:12:08Z

Library/Homebrew/cask/cask_loader.rb

@@ -235,31 +229,17 @@ def load(config:)
        json_cask = @from_json || Homebrew::API::Cask.all_casks[token]
        cask_source = JSON.pretty_generate(json_cask)

-        json_cask = Homebrew::API.merge_variations(json_cask).deep_symbolize_keys
-        tap = Tap.fetch(json_cask[:tap]) if json_cask[:tap].to_s.include?("/")
+        json_cask = Homebrew::API.merge_variations(json_cask).deep_symbolize_keys.freeze


This is just a nice to have thing.

MikeMcQuaid

Nice cleanup! Makes sense to me but would like @Bo98 and/or @Rylan12 to 👍🏻 it first.

BrewTestBot · 2023-02-28T00:06:15Z

Review period ended.

Bo98 · 2023-02-28T05:11:46Z

Didn't have a chance to have a proper look today but concept sounds good.

This overlaps a bit with what I was doing but I hadn't touched any of the cask side yet, so this will be a useful.

Rylan12

This looks great, thanks! My only question is about how loading from the source api works when loading an installed cask. Will this overwrite the cask file that was downloaded at the time of installation with the new, current source information? My understanding is that we always write the cask source as-is to the .metadata directory, so when loading from the installed cask file I'm not sure we ever need to redownload.

(Sorry, reviewing on mobile so not able to test it out)

Rylan12 · 2023-02-27T21:28:40Z

Library/Homebrew/cask/installer.rb

+    def load_cask_from_source_api!
+      options = { git_head: @cask.tap_git_head, sha256: @cask.ruby_source_checksum["sha256"] }
+      cask_source = Homebrew::API::Cask.fetch_source(@cask.token, **options)
+      @cask = CaskLoader::FromContentLoader.new(cask_source).load(config: @cask.config)


Do we need to include this to make sure that tap information is properly accounted for?

Suggested change

@cask = CaskLoader::FromContentLoader.new(cask_source).load(config: @cask.config)

@cask = CaskLoader::FromContentLoader.new(cask_source, tap: @cask.tap).load(config: @cask.config)

The tap doesn't seem to be recorded as metadata for the cask when it gets installed unless I'm missing something. That being said it is referenced once when deciding whether to track install analytics so it probably makes sense to add that logic back in (I had already removed it).

brew/Library/Homebrew/cask/installer.rb

Lines 113 to 115 in 6db7732

if @cask.tap&.should_report_analytics?

::Utils::Analytics.report_event(:cask_install, @cask.token, on_request: true)

end

Addressed in b91e93c

apainintheneck · 2023-02-28T06:59:07Z

This looks great, thanks! My only question is about how loading from the source api works when loading an installed cask. Will this overwrite the cask file that was downloaded at the time of installation with the new, current source information? My understanding is that we always write the cask source as-is to the .metadata directory, so when loading from the installed cask file I'm not sure we ever need to redownload.

(Sorry, reviewing on mobile so not able to test it out)

You're right about that. We first try to load from the installed cask file and only fallback to the source API if that's unavailable, corrupted or outdated and so can't be loaded correctly. We don't want to fallback to the JSON loaded cask in this specific case.

This info is used in the installer to decide whether to report analytics.

apainintheneck · 2023-02-28T08:14:46Z

Library/Homebrew/cask/cask_loader.rb

-      attr_reader :tap
-
      def initialize(path)
        @tap = Tap.from_path(path)
        super(path)
      end
-
-      private
-
-      def cask(*args, &block)
-        super(*args, tap: tap, &block)
-      end


We can remove these parts because @tap is now defined in the FromContentLoader and passed into the cask constructor from there.

Easy to verify by doing the following. Note that the FromDefaultTapPathLoader inherits from the FromTapPathLoader.

/u/l/Homebrew (delay-loading-from-cask-source-api|✔) $ brew irb ==> Interactive Homebrew Shell Example commands available with: `brew irb --examples` irb(main):001:0> cask = Cask::CaskLoader::FromDefaultTapPathLoader.new("virtualbox").load(config: nil); nil => nil irb(main):002:0> pp cask.tap; nil #<Tap:0x00007f9ff2a13530 @alias_reverse_table=nil, @alias_table=nil, @cask_dir= #<Pathname:/usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks>, @full_name="Homebrew/homebrew-cask", @name="homebrew/cask", @path=#<Pathname:/usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask>, @repo="cask", @user="Homebrew"> => nil

MikeMcQuaid · 2023-02-28T11:50:14Z

Thanks again @apainintheneck!

apainintheneck added 2 commits February 25, 2023 20:54

Add installer tests

9af04fd

apainintheneck added the cask Homebrew Cask label Feb 26, 2023

apainintheneck self-assigned this Feb 26, 2023

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

apainintheneck mentioned this pull request Feb 26, 2023

cask: to_h without api #14713

Merged

7 tasks

Merge branch 'master' into delay-loading-from-cask-source-api

fdc2e9f

apainintheneck marked this pull request as ready for review February 27, 2023 00:57

apainintheneck commented Feb 27, 2023

View reviewed changes

MikeMcQuaid requested review from Bo98 and Rylan12 and removed request for Bo98 February 27, 2023 13:33

MikeMcQuaid approved these changes Feb 27, 2023

View reviewed changes

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

BrewTestBot approved these changes Feb 28, 2023

View reviewed changes

Rylan12 approved these changes Feb 28, 2023

View reviewed changes

Load cask source with tap info

b91e93c

This info is used in the installer to decide whether to report analytics.

apainintheneck commented Feb 28, 2023

View reviewed changes

MikeMcQuaid merged commit d0e03fc into Homebrew:master Feb 28, 2023

apainintheneck deleted the delay-loading-from-cask-source-api branch March 24, 2023 03:25

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

github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay loading from cask source api #14820

Delay loading from cask source api #14820

apainintheneck commented Feb 26, 2023 •

edited

BrewTestBot commented Feb 26, 2023

apainintheneck Feb 27, 2023

apainintheneck Feb 27, 2023

apainintheneck Feb 27, 2023

MikeMcQuaid left a comment

BrewTestBot commented Feb 28, 2023

Bo98 commented Feb 28, 2023

Rylan12 left a comment

Rylan12 Feb 27, 2023

apainintheneck Feb 28, 2023

apainintheneck Feb 28, 2023

apainintheneck commented Feb 28, 2023

apainintheneck Feb 28, 2023

MikeMcQuaid commented Feb 28, 2023

	@cask = CaskLoader::FromContentLoader.new(cask_source).load(config: @cask.config)
	@cask = CaskLoader::FromContentLoader.new(cask_source, tap: @cask.tap).load(config: @cask.config)

	if @cask.tap&.should_report_analytics?
	::Utils::Analytics.report_event(:cask_install, @cask.token, on_request: true)
	end

Delay loading from cask source api #14820

Delay loading from cask source api #14820

Conversation

apainintheneck commented Feb 26, 2023 • edited

BrewTestBot commented Feb 26, 2023

apainintheneck Feb 27, 2023

Choose a reason for hiding this comment

apainintheneck Feb 27, 2023

Choose a reason for hiding this comment

apainintheneck Feb 27, 2023

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

BrewTestBot commented Feb 28, 2023

Bo98 commented Feb 28, 2023

Rylan12 left a comment

Choose a reason for hiding this comment

Rylan12 Feb 27, 2023

Choose a reason for hiding this comment

apainintheneck Feb 28, 2023

Choose a reason for hiding this comment

apainintheneck Feb 28, 2023

Choose a reason for hiding this comment

apainintheneck commented Feb 28, 2023

apainintheneck Feb 28, 2023

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 28, 2023

apainintheneck commented Feb 26, 2023 •

edited