Load internal cask json v3 #16896
There are no files selected for viewing
Codecov 
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -299,7 +299,12 @@ def load(config:) | |
|
|
||
| json_cask = Homebrew::API.merge_variations(json_cask).deep_symbolize_keys.freeze | ||
|
|
||
| cask_options[:tap] = if json_cask.key?(:tap) # public JSON v2 | ||
| tap_value = json_cask.fetch(:tap) | ||
| Tap.fetch(tap_value) if tap_value.to_s.include?("/") | ||
| else # internal JSON v3 | ||
| CoreCaskTap.instance | ||
| end | ||
|
Comment on lines
-302
to
+307
There was a problem hiding this comment. This is just me trying to avoid using There was a problem hiding this comment. I wonder if it makes sense to add the tap string to the There was a problem hiding this comment. I'm not sure either of them are really necessary for uninstalls though. There was a problem hiding this comment. It'd be good to figure out what the bare minimum is that we actually need stored here for e.g. uninstalls and just store that, probably as a new dedicated JSON file that bears no resemblance to public or private APIs and never store the cask file. There was a problem hiding this comment. I don't think it makes sense to work on and have to maintain another format. We should be good as long as tap is included so maybe we should just add it in the be sure about things. I'm going to think a bit more about how we want to handle loading legacy cask JSON that didn't have the tap defined. My first thought is that if we're loading from a JSON blob or file we should just use the defined tap if it exists.When loading from the API it should probably just default to the core cask tap. There was a problem hiding this comment.
I don't think it makes sense to store a bunch of data here that's not actually used and reuse/tweak an existing format when we already have two different "formats" (cask file vs. JSON on disk). Note that this has been a pretty long-running issue in Cask due to writing too much here and breaking backwards compatibility in the past causing Casks to not be able to be uninstalled. What I'm arguing for is essentially:
Genuinely I apologise that I keep stumbling on things that end up blowing up the scope here, it's just that some of this stuff has been very painful over long periods of time in Homebrew and I'd rather we did it properly if we're going to introduce a new API for this. Genuinely appreciate all your work here and elsewhere: thank you @apainintheneck ❤️
This seems reasonable. I can't see when, with the existing code, how JSON would be used for non-core cask taps though, right? It'll always use the Cask file I'd think. There was a problem hiding this comment. So what would the one file type be in the case that we decided to only support one format for installed casks? We could download the original cask file whenever we install a cask and just store that. Then, we wouldn't have to support the installed JSON file at all. Or we could generate a JSON file for every cask install and only store the JSON representation. There was a problem hiding this comment.
A JSON file.
A minimal JSON file that's a subset of internal JSON v3 containing only the information needed to e.g. uninstall a cask. |
||
|
|
||
| user_agent = json_cask.dig(:url_specs, :user_agent) | ||
| json_cask[:url_specs][:user_agent] = user_agent[1..].to_sym if user_agent && user_agent[0] == ":" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| RSpec.describe "Internal Tap JSON -- Cask" do | ||
| let(:internal_tap_json) { File.read(TEST_FIXTURE_DIR/"internal_tap_json/homebrew-cask.json").chomp } | ||
| let(:tap_git_head) { "b26c1e550a8b7eed2dcd5306ea8f3da3848258b3" } | ||
|
|
||
| context "when generating JSON", :needs_macos do | ||
| before do | ||
| FileUtils.rm_rf CoreCaskTap.instance.path | ||
| cp_r(TEST_FIXTURE_DIR/"internal_tap_json/homebrew-cask", Tap::TAP_DIRECTORY/"homebrew") | ||
| allow(Cask::Cask).to receive(:generating_hash?).and_return(true) | ||
| end | ||
|
|
||
| it "creates the expected hash" do | ||
| api_hash = CoreCaskTap.instance.to_internal_api_hash | ||
| api_hash["tap_git_head"] = tap_git_head # tricky to mock | ||
|
|
||
| expect(JSON.pretty_generate(api_hash)).to eq(internal_tap_json) | ||
| end | ||
| end | ||
|
|
||
| context "when loading JSON" do | ||
| before do | ||
| ENV["HOMEBREW_INTERNAL_JSON_V3"] = "1" | ||
| ENV.delete("HOMEBREW_NO_INSTALL_FROM_API") | ||
|
|
||
| allow(Homebrew::API).to receive(:fetch_json_api_file) | ||
| .with("internal/v3/homebrew-cask.jws.json") | ||
| .and_return([JSON.parse(internal_tap_json), false]) | ||
|
|
||
| # `Tap.tap_migration_oldnames` looks for renames in every | ||
| # tap so `CoreTap.tap_migrations` gets called and tries to | ||
| # fetch stuff from the API. This just avoids errors. | ||
| allow(Homebrew::API).to receive(:fetch_json_api_file) | ||
| .with("internal/v3/homebrew-core.jws.json") | ||
| .and_return([{ "tap_migrations" => {}, "formulae" => {}, "aliases" => {} }, false]) | ||
|
|
||
| # To allow `cask_names.txt` to be written to the cache. | ||
| (HOMEBREW_CACHE/"api").mkdir | ||
|
|
||
| Homebrew::API::Cask.clear_cache | ||
| end | ||
|
|
||
| it "loads cask renames" do | ||
| expect(CoreCaskTap.instance.cask_renames).to eq({ | ||
| "ankerslicer" => "ankermake", | ||
| "autodesk-fusion360" => "autodesk-fusion", | ||
| "betterdummy" => "betterdisplay", | ||
| "factor-lang" => "factor", | ||
| "smlnj-lang" => "smlnj", | ||
| }) | ||
| end | ||
|
|
||
| it "loads tap migrations" do | ||
| expect(CoreCaskTap.instance.tap_migrations).to eq({ | ||
| "azure-cli" => "homebrew/core", | ||
| "basex" => "homebrew/core", | ||
| "borgbackup" => "homebrew/core", | ||
| "chronograf" => "homebrew/core", | ||
| "consul" => "homebrew/core", | ||
| }) | ||
| end | ||
|
|
||
| it "loads tap git head" do | ||
| expect(Homebrew::API::Cask.tap_git_head) | ||
| .to eq(tap_git_head) | ||
| end | ||
|
|
||
| context "when loading formulae" do | ||
| let(:factor_metadata) do | ||
| { | ||
| "token" => "factor", | ||
| "name" => %w[Factor], | ||
| "desc" => "Programming language", | ||
| "homepage" => "https://factorcode.org/", | ||
| "version" => "0.99", | ||
| "ruby_source_path" => "Casks/f/factor.rb", | ||
| "ruby_source_checksum" => { | ||
| "sha256" => "a0dabe24c67269e5310b47639cf32e74f49959ba1be454b2c072805b1f04c7e5", | ||
| }, | ||
| } | ||
| end | ||
|
|
||
| let(:smlnj_metadata) do | ||
| { | ||
| "token" => "smlnj", | ||
| "name" => ["Standard ML of New Jersey"], | ||
| "desc" => "Compiler for the Standard ML '97 programming language", | ||
| "homepage" => "https://www.smlnj.org/", | ||
| "version" => "110.99.4", | ||
| "ruby_source_path" => "Casks/s/smlnj.rb", | ||
| "ruby_source_checksum" => { | ||
| "sha256" => "d47f46a88248272314a501741460d42a8c731030912a83ef58d3c7fd1e90034d", | ||
| }, | ||
| } | ||
| end | ||
|
|
||
| it "loads factor" do | ||
| factor = Cask::CaskLoader.load("factor") | ||
| expect(factor.to_h).to include(factor_metadata) | ||
| expect(factor.sha256).to eq("8a7968b873b5e87c83b5d0f5ddb4d3d76a2460f5e5c14edac6b18fe5957bd7d6") | ||
| expect(factor.url.to_s).to eq("https://downloads.factorcode.org/releases/0.99/factor-macosx-x86-64-0.99.dmg") | ||
| end | ||
|
|
||
| it "loads factor from rename" do | ||
| factor = Cask::CaskLoader.load("factor-lang") | ||
| expect(factor.to_h).to include(**factor_metadata) | ||
| end | ||
|
|
||
| it "loads smlnj" do | ||
| smlnj = Cask::CaskLoader.load("smlnj") | ||
| expect(smlnj.to_h).to include(**smlnj_metadata) | ||
| expect(smlnj.sha256).to eq("2bf858017b8ba43a70b30527290ed9fbbc81d9eaac1abeba62469d95392019a3") | ||
| expect(smlnj.url.to_s).to eq("http://smlnj.cs.uchicago.edu/dist/working/110.99.4/smlnj-amd64-110.99.4.pkg") | ||
| end | ||
|
|
||
| it "loads smlnj from rename" do | ||
| smlnj = Cask::CaskLoader.load("smlnj-lang") | ||
| expect(smlnj.to_h).to include(**smlnj_metadata) | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| { | ||
| "tap_git_head": "b26c1e550a8b7eed2dcd5306ea8f3da3848258b3", | ||
| "renames": { | ||
| "ankerslicer": "ankermake", | ||
| "autodesk-fusion360": "autodesk-fusion", | ||
| "betterdummy": "betterdisplay", | ||
| "factor-lang": "factor", | ||
| "smlnj-lang": "smlnj" | ||
| }, | ||
| "tap_migrations": { | ||
| "azure-cli": "homebrew/core", | ||
| "basex": "homebrew/core", | ||
| "borgbackup": "homebrew/core", | ||
| "chronograf": "homebrew/core", | ||
| "consul": "homebrew/core" | ||
| }, | ||
| "casks": { | ||
| "factor": { | ||
| "name": [ | ||
| "Factor" | ||
| ], | ||
| "desc": "Programming language", | ||
| "homepage": "https://factorcode.org/", | ||
| "url": "https://downloads.factorcode.org/releases/0.99/factor-macosx-x86-64-0.99.dmg", | ||
| "version": "0.99", | ||
| "sha256": "8a7968b873b5e87c83b5d0f5ddb4d3d76a2460f5e5c14edac6b18fe5957bd7d6", | ||
| "artifacts": [ | ||
| { | ||
| "suite": [ | ||
| "factor" | ||
| ] | ||
| } | ||
| ], | ||
| "ruby_source_path": "Casks/f/factor.rb", | ||
| "ruby_source_sha256": "a0dabe24c67269e5310b47639cf32e74f49959ba1be454b2c072805b1f04c7e5", | ||
| "caveats": "To use factor, you may need to add the $APPDIR/factor directory\nto your PATH environment variable, e.g. (for Bash shell):\n export PATH=$APPDIR/factor:\"$PATH\"\n" | ||
| }, | ||
| "smlnj": { | ||
| "name": [ | ||
| "Standard ML of New Jersey" | ||
| ], | ||
| "desc": "Compiler for the Standard ML '97 programming language", | ||
| "homepage": "https://www.smlnj.org/", | ||
| "url": "http://smlnj.cs.uchicago.edu/dist/working/110.99.4/smlnj-amd64-110.99.4.pkg", | ||
| "version": "110.99.4", | ||
| "sha256": "2bf858017b8ba43a70b30527290ed9fbbc81d9eaac1abeba62469d95392019a3", | ||
| "artifacts": [ | ||
| { | ||
| "uninstall": [ | ||
| { | ||
| "pkgutil": "org.smlnj.amd64.pkg" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "pkg": [ | ||
| "smlnj-amd64-110.99.4.pkg" | ||
| ] | ||
| }, | ||
| { | ||
| "zap": [ | ||
| { | ||
| "delete": "/usr/local/smlnj" | ||
| } | ||
| ] | ||
| } | ||
| ], | ||
| "ruby_source_path": "Casks/s/smlnj.rb", | ||
| "ruby_source_sha256": "d47f46a88248272314a501741460d42a8c731030912a83ef58d3c7fd1e90034d", | ||
| "url_specs": { | ||
| "verified": "smlnj.cs.uchicago.edu/" | ||
| }, | ||
| "caveats": "To use smlnj, you may need to add the /usr/local/smlnj/bin directory\nto your PATH environment variable, e.g. (for Bash shell):\n export PATH=/usr/local/smlnj/bin:\"$PATH\"\n" | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| cask "factor" do | ||
| version "0.99" | ||
| sha256 "8a7968b873b5e87c83b5d0f5ddb4d3d76a2460f5e5c14edac6b18fe5957bd7d6" | ||
|
|
||
| url "https://downloads.factorcode.org/releases/#{version}/factor-macosx-x86-64-#{version}.dmg" | ||
| name "Factor" | ||
| desc "Programming language" | ||
| homepage "https://factorcode.org/" | ||
|
|
||
| livecheck do | ||
| url "https://downloads.factorcode.org/releases/" | ||
| regex(%r{href=.*?(\d+(?:\.\d+)+)/}i) | ||
| end | ||
|
|
||
| suite "factor" | ||
|
|
||
| caveats do | ||
| path_environment_variable "#{appdir}/factor" | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up removing this TODO because we won't be able to remove this going forward. We need to keep backwards compatibility with all of the JSON cask files that get stored when casks get installed from the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apainintheneck Oof. That seems undesirable (but clearly not your fault). Could we add some sort of versioning to these files/filenames so this isn't going to have to be the case indefinitely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll have to keep backwards compatibility until all of the currently supported macOS versions reach end of life. It will be a while. I'm not sure how we handle very long term deprecations like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apainintheneck That's not tenable given how recently this was introduced and that we have e.g. already two paths for this (writing formula files, writing JSON files). Instead, we should handle when this JSON file is in an invalid format and act the same as if it were missing entirely (which is something that already works).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that is another option. We don't even need to explicitly handle it being in an invalid format since as you already said we handle that already. I guess in that case it shouldn't be that big of a deal.
brew/Library/Homebrew/cask/installer.rb
Lines 576 to 593 in 7b2bfee
We already fall back to the current cask file whenever the old cask file can't be loaded. I still think backwards compatibility is cheap here though and I don't understand why we would want to break it. Breaking backwards compatibility here could potentially affect all casks installed in the last year from the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is "this", sorry?
This seems reasonable but I don't care enough to argue about it any more if you feel strongly about this.
The alternative is: write a JSON file into the Caskroom in the same format whether installing from homebrew-cask, other official taps or a 3rd-party tap. Again: the status quo of "install caskfile sometimes and JSON file other times" is not a good one.
If it was all neatly sorted into methods or a class/classes named accordingly: no big deal. As-is: it's going to be hard to extract later.
Again: the more conservative the approaches to changing the JSON, the less it seems worthy of having an internal JSON and/or a v3 JSON at all.
To be clear: I think the v3 JSON is very desirable and the internal one is: we just need to do it right. This is an API format that may still be with us in 5-10 years.
If it's in a single location that's easier to delete/remove/deprecate later: that's better than having it inline.
See the OS layer as an example: it makes it much easier than just having
if OS.mac?scattered all over the place.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I was talking about the idea of having a reduced info cask JSON that could only be used to uninstall casks.
The cask JSON doesn't support all of the features found in the cask Ruby file right now so I don't think we'll be able to do that unless we change how the Ruby file works.
Fair point. 👍
It'll still be smaller and more efficient just not as pretty.
That makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. You've convinced me, thanks for taking the time to do so ❤️
What regressions have we caused to users through not writing cask files to the Caskroom? If there are (m)any: have there been (m)any user reports about this (given that most users have been using this for over a year)?
(implication being: few/zero user complaints about this means that this is not actually a meaningful problem for users worth blocking moving everyone to this path)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to the casks which are
Cask::Cask#caskfile_only?. They either have languages or *flight blocks. It hasn't caused any problems because we currently sidestep the problem completely and just download the original cask file (the Ruby one) whenever we install them.HOMEBREW_INSTALL_FROM_API#14304languageswhen installing from the API #14456There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, forgot about that.
We should probably figure out how to add the necessary data to JSON v3.
I know, scope creep, sorry.