From 374f788a3c6a8f295de4e5eb134d979eb3dc3513 Mon Sep 17 00:00:00 2001 From: Claudia Date: Tue, 21 Apr 2020 17:39:07 +0200 Subject: [PATCH 1/2] cask/config_spec: add failing test We recently added a new global artifact and then updated a cask to make use of that new artifact. This caused a number of `brew cask` commands to fail for users who had the cask installed before the artifact was added. This commit adds test cases for that failure mode. See also: - https://github.com/Homebrew/brew/pull/7286#issuecomment-613376568 - https://discourse.brew.sh/t/cask-definition-is-invalid-invalid-mdimporter-stanza-key-not-found-mdimporterdir --- Library/Homebrew/test/cask/config_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Library/Homebrew/test/cask/config_spec.rb b/Library/Homebrew/test/cask/config_spec.rb index f7cc6aebc0..f968f533c6 100644 --- a/Library/Homebrew/test/cask/config_spec.rb +++ b/Library/Homebrew/test/cask/config_spec.rb @@ -50,4 +50,20 @@ describe Cask::Config, :cask do expect(config.explicit[:appdir]).to eq(Pathname("/explicit/path/to/apps")) end end + + context "when installing a cask and then adding a global default dir" do + let(:config) { described_class.new(default: { appdir: "/default/path/before/adding/fontdir" }) } + + describe "#appdir" do + it "honors metadata of the installed cask" do + expect(config.appdir).to eq(Pathname("/default/path/before/adding/fontdir")) + end + end + + describe "#fontdir" do + it "falls back to global default on incomplete metadata" do + expect(config.default).to include(fontdir: Pathname(TEST_TMPDIR).join("cask-fontdir")) + end + end + end end From d026cb91e7f59c7eff32f798b4c902e6c50ce122 Mon Sep 17 00:00:00 2001 From: Claudia Date: Tue, 21 Apr 2020 17:45:34 +0200 Subject: [PATCH 2/2] Fix cask loading after adding an artifact type This commit fixes an issue where we added a new global artifact and then updated a cask to make use of that new artifact. This caused a number of `brew cask` commands to fail for users who had the cask installed before the artifact was added. When loading the definition of an installed cask, we configure it using a snapshot from install time, e. g. `/usr/local/Caskroom/markdownmdimporter/.metadata/config.json`. The snapshot looks like this: ``` { "default": { "appdir": "/Applications", "prefpanedir": "/Users/claudia/Library/PreferencePanes", "qlplugindir": "/Users/claudia/Library/QuickLook", "dictionarydir": "/Users/claudia/Library/Dictionaries", "fontdir": "/Users/claudia/Library/Fonts", "colorpickerdir": "/Users/claudia/Library/ColorPickers", "servicedir": "/Users/claudia/Library/Services", "input_methoddir": "/Users/claudia/Library/Input Methods", "internet_plugindir": "/Users/claudia/Library/Internet Plug-Ins", "audio_unit_plugindir": "/Users/claudia/Library/Audio/Plug-Ins/Components", "vst_plugindir": "/Users/claudia/Library/Audio/Plug-Ins/VST", "vst3_plugindir": "/Users/claudia/Library/Audio/Plug-Ins/VST3", "screen_saverdir": "/Users/claudia/Library/Screen Savers" }, "env": {}, "explicit": {} } ``` Note that there is no `mdimporterdir` because the cask was installed before the artifact was added. The root cause is that the cask loading code still expects the snapshot to contain directory configuration for all artifact types. Since the snapshot never learned about the new artifact type, cask loading would fail. The fix applied in this commit is to fall back to the global default whenever the `default` directory map of a configuration snapshot is incomplete. See also: - https://github.com/Homebrew/brew/pull/7286#issuecomment-613376568 - https://discourse.brew.sh/t/cask-definition-is-invalid-invalid-mdimporter-stanza-key-not-found-mdimporterdir --- Library/Homebrew/cask/config.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/config.rb b/Library/Homebrew/cask/config.rb index 12d2c96586..6ddad5430b 100644 --- a/Library/Homebrew/cask/config.rb +++ b/Library/Homebrew/cask/config.rb @@ -69,7 +69,7 @@ module Cask attr_accessor :explicit def initialize(default: nil, env: nil, explicit: {}) - @default = self.class.canonicalize(default) if default + @default = DEFAULT_DIRS.merge(self.class.canonicalize(default)) if default @env = self.class.canonicalize(env) if env @explicit = self.class.canonicalize(explicit)