Properly upgrade Casks with version :latest #3492

amyspark · 2017-11-28T00:38:54Z

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 tests with your changes locally?

When I proposed #3396, I forgot to test if the upgrade process worked for version :latest Casks.
Obviously, this came back to bite me 😢

In this initial state, I've submitted a test to summarize the issue. The missing part would be to tamper with staged_path if version :latest and use a randomized? path, but I'd like to request your comments on the best way to do it.

This pull request, once finished, fixes Homebrew/homebrew-cask#41373.

Thanks for your comments!

/cc @MikeMcQuaid @vitorgalvao @reitermarkus

reitermarkus · 2017-11-28T01:28:42Z

Obviously, this came back to bite me 😢

No worries, there had to be something. 😜

and use a randomized? path

brew reinstall appends .reinstall to the Keg path: https://github.com/Homebrew/brew/blob/master/Library/Homebrew/cmd/reinstall.rb#L74-L76

I'd suggest to just move staged_path to "#{staged_path}.backup" (or .upgrading), and either remove it after the upgrade, or move it back if the upgrade fails.

reitermarkus · 2017-11-28T15:59:09Z

Library/Homebrew/cask/lib/hbc/installer.rb

@@ -420,6 +436,14 @@ def zap
      purge_caskroom_path
    end

+    def backup_path(path)
+      Pathname.new "#{path}.upgrading" unless path.nil?


When would path be nil?

Also, since this is only used for staged_path, you could make this take no arguments and just return Pathname("#{@cask.staged_path}.upgrading").

path should never be nil, but I wanted to preserve the semantics of gain_permissions_remove(staged_path) if !staged_path.nil? && staged_path.exist?.

reitermarkus · 2017-11-28T16:03:15Z

Library/Homebrew/cask/lib/hbc/installer.rb

@@ -378,10 +378,26 @@ def start_upgrade

      disable_accessibility_access
      uninstall_artifacts
+      backup if version_is_latest?


Let's always create a backup, not just when version_is_latest?.

If we always create a backup, we'll need a way to know if this is the new or old cask when purge_versioned_files. Otherwise, it will leave a dangling, empty folder if the upgrade fails (see the test "restores the old Cask if the upgrade failed" in upgrade_spec.rb).
Would another variable e.g. old_cask, new_cask instead of upgrade work for you?

Isn't start_upgrade only called on the old cask?

Yes, I took advantage of that; see next commit.

(and if doing so, correctly set the staged_path when purging it)

reitermarkus · 2017-11-28T17:47:41Z

Library/Homebrew/cask/lib/hbc/installer.rb

+      if backed_up?
+        Pathname.new "#{@cask.staged_path}.upgrading"
+      else
+        @cask.staged_path


I think this is really hard to follow. The only reason for this is purge_versioned_files using it. I'd suggest making two specialised versions of purge_versioned_files, one normal one, and one for the upgrade? case.

I'll do it that way, and then factor out the whole metadata handling (to avoid code duplication).

Avoid using variables altogether; fork out the purge into two specialized + one common function

reitermarkus · 2017-11-28T19:04:52Z

Library/Homebrew/cask/lib/hbc/installer.rb

      if @cask.metadata_versioned_path.respond_to?(:children) &&
-         @cask.metadata_versioned_path.exist?
+         @cask.metadata_versioned_path.exist? &&
+         !(upgrade? && version_is_latest?)


Do we still need version_is_latest? now?

Yes, the backup mechanism doesn't involve the Cask's metadata; if the version is latest, deleting this line will make purge_metadata "uninstall" the Cask while leaving all artifacts in place.

On second thoughts, I'll just go ahead with two fully separate methods and get rid of that line.

reitermarkus · 2017-11-28T19:07:29Z

Library/Homebrew/cask/lib/hbc/installer.rb

    def gain_permissions_remove(path)
      Utils.gain_permissions_remove(path, command: @command)
    end

+    def purge_backed_versioned_files


purge_backed_up_versioned_files

reitermarkus · 2017-11-28T22:51:36Z

Library/Homebrew/cask/lib/hbc/installer.rb

+      Pathname.new "#{@cask.metadata_versioned_path}.upgrading"
+    end
+
+    def version_is_latest?


This can now be removed.

reitermarkus · 2017-11-28T22:54:58Z

Library/Homebrew/cask/lib/hbc/installer.rb

+         end
+      end
+      backup_metadata_path.rmdir_if_possible
+      @cask.metadata_master_container_path.rmdir_if_possible unless upgrade?


The unless upgrade? here doesn't make sense, because this method is only called when upgrading.

reitermarkus · 2017-11-28T23:08:04Z

Library/Homebrew/cask/lib/hbc/installer.rb

@@ -31,7 +31,7 @@ def initialize(cask, command: SystemCommand, force: false, skip_cask_deps: false
      @upgrade = upgrade
    end

-    attr_predicate :binaries?, :force?, :skip_cask_deps?, :require_sha?, :upgrade?, :verbose?
+    attr_predicate :binaries?, :force?, :skip_cask_deps?, :require_sha?, :upgrade?, :verbose?, :backed_up?


This is not needed anymore.

reitermarkus · 2017-11-29T00:56:17Z

Library/Homebrew/cask/lib/hbc/artifact/moved.rb

@@ -71,7 +71,7 @@ def move_back(skip: false, force: false, command: nil, **options)
          command.run("/bin/mv", args: [target, source], sudo: true)
        end

-        add_altname_metadata(target, source.basename, command: command)
+        add_altname_metadata(source, target.basename, command: command)


Remove this completely. We don‘t need to add this when moving it back, actually.

reitermarkus · 2017-11-29T00:59:15Z

Library/Homebrew/cask/lib/hbc/installer.rb

+
+      # Homebrew-Cask metadata
+      if backup_metadata_path.respond_to?(:children) &&
+         backup_metadata_path.exist? &&


The && shouldn‘t be here.

The whole condition here could probably just be backup_metadata_path.directory?.

Leftover && and remove xattr "copy"

reitermarkus · 2017-11-29T01:05:25Z

I think you missed this comment by a minute: #3492 (comment)

amyspark · 2017-11-29T01:10:22Z

Yep, I wasn't using my inbox to keep up with comments. Thanks for the heads up @reitermarkus.

Simplify a condition

reitermarkus · 2017-11-29T14:01:02Z

Thanks again, @amyspark! 👍

Test upgrade for Casks with version :latest

10a0662

reitermarkus added the cask Homebrew Cask label Nov 28, 2017

amyspark added 3 commits November 28, 2017 13:03

Stage to a different directory if upgrading Casks w/version :latest

f3355b2

Solve styling and metadata issues

8386b93

Use the proper path when purging metadata

2ff114b

amyspark mentioned this pull request Nov 28, 2017

Updated Visual Paradigm CE to version 14.2,20171107 Homebrew/homebrew-cask#41392

Merged

3 tasks

reitermarkus reviewed Nov 28, 2017

View reviewed changes

Always backup the old Cask prior to upgrade

9458e98

(and if doing so, correctly set the staged_path when purging it)

reitermarkus reviewed Nov 28, 2017

View reviewed changes

Try #2 at handling backed up Casks

9227c8f

Avoid using variables altogether; fork out the purge into two specialized + one common function

reitermarkus reviewed Nov 28, 2017

View reviewed changes

amyspark added 2 commits November 28, 2017 20:36

Rename purge_backed_versioned_files to purge_backed_up_versioned_files

5840b2f

Fully separate purge_versioned_files

63961ac

reitermarkus reviewed Nov 28, 2017

View reviewed changes

Remove unused backup and metadata logic

657d4ea

reitermarkus reviewed Nov 29, 2017

View reviewed changes

Implement @reitermarkus's comments

ab62be5

Leftover && and remove xattr "copy"

Leftover of @reitermarkus's comments

96187ad

Simplify a condition

reitermarkus approved these changes Nov 29, 2017

View reviewed changes

reitermarkus merged commit 3f7e63a into Homebrew:master Nov 29, 2017

amyspark deleted the fix-latest-casks branch November 29, 2017 14:37

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly upgrade Casks with version :latest #3492

Properly upgrade Casks with version :latest #3492

amyspark commented Nov 28, 2017 •

edited

Loading

reitermarkus commented Nov 28, 2017

reitermarkus Nov 28, 2017

reitermarkus Nov 28, 2017

amyspark Nov 28, 2017

reitermarkus Nov 28, 2017

amyspark Nov 28, 2017 •

edited

Loading

reitermarkus Nov 28, 2017

amyspark Nov 28, 2017

reitermarkus Nov 28, 2017

amyspark Nov 28, 2017

reitermarkus Nov 28, 2017

amyspark Nov 28, 2017

amyspark Nov 28, 2017

reitermarkus Nov 28, 2017

reitermarkus Nov 28, 2017

reitermarkus Nov 28, 2017

reitermarkus Nov 28, 2017

reitermarkus Nov 29, 2017

reitermarkus Nov 29, 2017

reitermarkus Nov 29, 2017

reitermarkus commented Nov 29, 2017 •

edited

Loading

amyspark commented Nov 29, 2017

reitermarkus commented Nov 29, 2017

Properly upgrade Casks with version :latest #3492

Properly upgrade Casks with version :latest #3492

Conversation

amyspark commented Nov 28, 2017 • edited Loading

reitermarkus commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyspark Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus commented Nov 29, 2017 • edited Loading

amyspark commented Nov 29, 2017

reitermarkus commented Nov 29, 2017

amyspark commented Nov 28, 2017 •

edited

Loading

amyspark Nov 28, 2017 •

edited

Loading

reitermarkus commented Nov 29, 2017 •

edited

Loading