Don't remove cask directories when upgrading. #15138

JBYoshi · 2023-04-03T23:58:39Z

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?

When updating casks, Homebrew deletes the old version of the application before moving the new one in its place. This causes macOS to think that the application was uninstalled, and so it may get removed from places like notification settings and Launchpad. This PR changes the move behavior for cask directories so the app directory itself is retained during the upgrade, while the contents are replaced.

Fixes Homebrew/homebrew-cask#102721.

reitermarkus · 2023-04-04T00:42:46Z

Okay, so instead of essentially rm -r App.app, we only rm -r App.app/* and then replace the contents?

Do you have an example cask that breaks if you remove it completely to test this?

JBYoshi · 2023-04-04T01:38:38Z

That's the gist of it.

I haven't figured out an automated test case yet since I don't know of any APIs that Apple exposes for the dock / Launchpad. However, I have been manually testing using the 1password cask. Here's my current procedure:

Add a sleep(10) call to the end of Cask::Artifact::Moved::delete(). (This accounts for the timing sensitivity discussed in the original issue.)
Install an old version of the 1password cask (I used 8.10.1).
Pin the 1Password app to the dock and/or add it to a folder in Launchpad.
Change the cask file to a newer version (I used 8.10.3).
Run brew upgrade.

With the original code, the 1Password app will disappear from the dock and will be moved out of the folder in Launchpad. With this code, the app remains in the dock and the Launchpad folder.

One possible automated test case could be making sure the inode number stays the same before and after the upgrade. That wouldn't check Launchpad or the dock directly but it will ensure the directory isn't deleted.

reitermarkus · 2023-04-04T01:46:41Z

Is copying xattrs crucial for this to work? Do we care about any xattrs apart from the quarantine attribute?

MikeMcQuaid

The code seems reasonable to me but will wait for another @Homebrew/cask 👍🏻/✅ first.

If this stops the "multiple VSCode icons in my toolbar" issues: I will be very grateful.

bevanjkay · 2023-04-04T11:28:52Z

@MikeMcQuaid Unfortunately this won't solve the issue with Visual Studio Code. There's something with the architecture that allows multiple instances of the app to be running, and when two different versions of the app are open, two icons are present in dock. I have checked out this branch and will see how it goes over the next day or so with these changes and report back.

bevanjkay · 2023-04-04T11:34:17Z

I can confirm that this is working as expected with a cask that is known to have this issue (Spotify).

A couple of questions;

Should the same logic apply with brew reinstall <token>?
Should the xattr be copied across? Should the user only be notified about the source when they are opening and upgraded app for the first time? My assumption is that we should expect the notification whenever the binary/version changes for security purposes?

“Spotify” is an app downloaded from the internet. Are you sure you want to open it?

JBYoshi · 2023-04-04T15:29:07Z

For the xattrs part, the original behavior includes copying xattrs (link), so I kept that for consistency.

brew reinstall might be a good idea to support as well. I'll look into that later today.

p-linnane

Would love if this resolves our long standing issue. Thanks @JBYoshi!

JBYoshi · 2023-04-04T16:39:52Z

I've changed the upgrade parameter on artifacts to upgrade_or_reinstall so this behavior should also apply to reinstalls. I'll note that the upgrade parameter was also used for login item uninstallation, but I think it would also make sense to use this behavior for login item reinstalls, so I've pulled that completely into the upgrade_or_reinstall behavior.

reitermarkus

I think there is an edge case that still needs to be handled here:

If an app is renamed, will this leave an empty directory for the old app name?

SMillerDev · 2023-04-05T16:31:25Z

Does this work with codesigning too? Otherwise it'll disable any upgraded app on ARM.

bevanjkay · 2023-04-05T22:58:38Z

@SMillerDev I have been using this branch on my ARM MacBook for the last couple of days and all seems to be well.

JBYoshi · 2023-04-08T21:14:17Z

I think there is an edge case that still needs to be handled here:

If an app is renamed, will this leave an empty directory for the old app name?

Good point. I've added some behavior to handle that by passing in the old and new versions of the casks to the artifact installation and uninstallation steps.

apainintheneck · 2023-04-14T03:07:15Z

Looks like this needs to be updated to reflect the changes made to cask/reinstall in #15184.

JBYoshi · 2023-04-16T02:38:36Z

Updated to fix the merge conflicts.

MikeMcQuaid · 2023-04-17T11:19:48Z

Fine with this when @reitermarkus is.

JBYoshi · 2023-04-23T18:57:57Z

Any updates on this?

reitermarkus

Some small suggestions, but should be ready to merge after that.

Maybe the artifacts.any? part can also be moved into a helper method since it's the same for move and delete.

reitermarkus · 2023-04-23T19:46:40Z

Library/Homebrew/cask/artifact/moved.rb

        ohai "Removing #{self.class.english_name} '#{target}'"
        raise CaskError, "Cannot remove undeletable #{self.class.english_name}." if MacOS.undeletable?(target)

        return unless Utils.path_occupied?(target)

-        if target.parent.writable? && !force
+        if !successor.nil? && target.directory? && successor.artifacts.any? do |a|
+             a.class.dsl_key == self.class.dsl_key && a.target == self.target


Suggested change

a.class.dsl_key == self.class.dsl_key && a.target == self.target

a.class == self.class && a.target == self.target

Updated, with one caveats from the style checker. It doesn't like an equality test on the classes, so I used a two-way instance_of? check that has the same behavior.

Library/Homebrew/cask/artifact/moved.rb

Library/Homebrew/cask/installer.rb

Library/Homebrew/cask/artifact/moved.rb

reitermarkus

One small change still needed, otherwise looks ready to merge.

JBYoshi · 2023-04-30T17:30:27Z

Updated for that feedback.

carlocab

Now that there's a new release tag, we should get this in ASAP so this has time to bake.

MikeMcQuaid

Looks good! Some style suggestions.

Library/Homebrew/cask/installer.rb

Library/Homebrew/cask/quarantine.rb

Library/Homebrew/cask/utils/copy-xattrs.swift

JBYoshi · 2023-05-03T16:29:38Z

@MikeMcQuaid Done.

MikeMcQuaid · 2023-05-04T10:43:57Z

Thanks so much for your first, great contribution (hopefully of many)! Without people like you submitting PRs we couldn't run this project. You rock, @JBYoshi!

SMillerDev · 2023-05-10T15:45:57Z

Could this be causing https://github.com/orgs/Homebrew/discussions/4498?

timfallmk · 2023-05-10T17:31:50Z

Could this be causing https://github.com/orgs/Homebrew/discussions/4498?

This is very likely related. I ran into the same problem when running a standard upgrade of chrome and chrome canary a day ago, but didn't see any reports of it at the time.

I'll see if I can dig some relevant logs back out.

Bo98 · 2023-05-10T18:37:22Z

I've merged #15399 which will improve the errors a bit (the stack trace will actually be useful now), but yeah will need to track down the source.

Apparently the issues might be Ventura-specific. I'm running Monterey at the moment.

SMillerDev · 2023-05-10T19:09:09Z

I'm running the beta, let me know if I can test something

xkia · 2023-05-18T07:38:55Z

On the Ventura, it is not possible to upgrade the application of cash normally

maxim-badarau-m10 · 2023-05-18T13:31:24Z

Any updates on the issue, it happen for me today

JBYoshi · 2023-05-24T23:03:54Z

I think these issues might have to do with the new "App Management" permissions in macOS Ventura. More info here: Homebrew/homebrew-cask#147383 (comment)

Don't remove cask directories when upgrading.

8c4f29d

Fixes Homebrew/homebrew-cask#102721.

reitermarkus added the cask Homebrew Cask label Apr 4, 2023

Reset extended attributes of the base directories.

3f8998a

JBYoshi marked this pull request as ready for review April 4, 2023 01:22

reitermarkus requested a review from a team April 4, 2023 01:46

MikeMcQuaid approved these changes Apr 4, 2023

View reviewed changes

Add unit test for upgrading.

5c9fa84

p-linnane approved these changes Apr 4, 2023

View reviewed changes

Also keep app directories with "brew reinstall".

dcc4ae7

reitermarkus requested changes Apr 5, 2023

View reviewed changes

Only keep empty directories when the new version also has them.

9fedaee

MikeMcQuaid requested a review from reitermarkus April 10, 2023 11:40

Merge remote-tracking branch 'origin/master' into cask-move-contents

2e85f2b

carlocab approved these changes Apr 16, 2023

View reviewed changes

reitermarkus approved these changes Apr 23, 2023

View reviewed changes

reitermarkus reviewed Apr 23, 2023

View reviewed changes

Library/Homebrew/cask/installer.rb Outdated Show resolved Hide resolved

Remove successor behavior for zapping.

84fe93e

reitermarkus reviewed Apr 30, 2023

View reviewed changes

Library/Homebrew/cask/artifact/moved.rb Outdated Show resolved Hide resolved

reitermarkus requested changes Apr 30, 2023

View reviewed changes

Remove unreachable warning.

4935a8f

carlocab approved these changes May 1, 2023

View reviewed changes

MikeMcQuaid requested a review from reitermarkus May 1, 2023 14:35

reitermarkus approved these changes May 2, 2023

View reviewed changes

reitermarkus requested a review from MikeMcQuaid May 2, 2023 22:12

MikeMcQuaid reviewed May 3, 2023

View reviewed changes

Improve styling.

3e249a9

MikeMcQuaid merged commit 0e387fe into Homebrew:master May 4, 2023

JBYoshi deleted the cask-move-contents branch May 4, 2023 15:46

JBYoshi mentioned this pull request May 11, 2023

Fix wildcard copy in cask updates. #15407

Merged

7 tasks

apainintheneck mentioned this pull request May 12, 2023

Error during brew update && brew upgrade && brew cleanup #15406

Closed

3 tasks

JBYoshi mentioned this pull request May 12, 2023

Fix typing on revert_upgrade(). #15412

Merged

7 tasks

xkia mentioned this pull request May 19, 2023

brew upgrade --cask wechat error Homebrew/homebrew-cask#147383

Closed

3 tasks

SMillerDev mentioned this pull request May 24, 2023

Can't update any cask. Problems with permissions Homebrew/homebrew-cask#147789

Closed

6 tasks

JBYoshi mentioned this pull request May 25, 2023

Check for App Management permissions before updating apps. #15483

Merged

7 tasks

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't remove cask directories when upgrading. #15138

Don't remove cask directories when upgrading. #15138

JBYoshi commented Apr 3, 2023 •

edited

Loading

reitermarkus commented Apr 4, 2023

JBYoshi commented Apr 4, 2023

reitermarkus commented Apr 4, 2023

MikeMcQuaid left a comment

bevanjkay commented Apr 4, 2023

bevanjkay commented Apr 4, 2023

JBYoshi commented Apr 4, 2023

p-linnane left a comment

JBYoshi commented Apr 4, 2023

reitermarkus left a comment

SMillerDev commented Apr 5, 2023

bevanjkay commented Apr 5, 2023

JBYoshi commented Apr 8, 2023

apainintheneck commented Apr 14, 2023

JBYoshi commented Apr 16, 2023

MikeMcQuaid commented Apr 17, 2023

JBYoshi commented Apr 23, 2023

reitermarkus left a comment

reitermarkus Apr 23, 2023

JBYoshi Apr 24, 2023

reitermarkus left a comment

JBYoshi commented Apr 30, 2023

carlocab left a comment

MikeMcQuaid left a comment

JBYoshi commented May 3, 2023

MikeMcQuaid commented May 4, 2023

SMillerDev commented May 10, 2023

timfallmk commented May 10, 2023

Bo98 commented May 10, 2023 •

edited

Loading

SMillerDev commented May 10, 2023

xkia commented May 18, 2023

maxim-badarau-m10 commented May 18, 2023

JBYoshi commented May 24, 2023

	a.class.dsl_key == self.class.dsl_key && a.target == self.target
	a.class == self.class && a.target == self.target

Don't remove cask directories when upgrading. #15138

Don't remove cask directories when upgrading. #15138

Conversation

JBYoshi commented Apr 3, 2023 • edited Loading

reitermarkus commented Apr 4, 2023

JBYoshi commented Apr 4, 2023

reitermarkus commented Apr 4, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

bevanjkay commented Apr 4, 2023

bevanjkay commented Apr 4, 2023

JBYoshi commented Apr 4, 2023

p-linnane left a comment

Choose a reason for hiding this comment

JBYoshi commented Apr 4, 2023

reitermarkus left a comment

Choose a reason for hiding this comment

SMillerDev commented Apr 5, 2023

bevanjkay commented Apr 5, 2023

JBYoshi commented Apr 8, 2023

apainintheneck commented Apr 14, 2023

JBYoshi commented Apr 16, 2023

MikeMcQuaid commented Apr 17, 2023

JBYoshi commented Apr 23, 2023

reitermarkus left a comment

Choose a reason for hiding this comment

reitermarkus Apr 23, 2023

Choose a reason for hiding this comment

JBYoshi Apr 24, 2023

Choose a reason for hiding this comment

reitermarkus left a comment

Choose a reason for hiding this comment

JBYoshi commented Apr 30, 2023

carlocab left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

JBYoshi commented May 3, 2023

MikeMcQuaid commented May 4, 2023

SMillerDev commented May 10, 2023

timfallmk commented May 10, 2023

Bo98 commented May 10, 2023 • edited Loading

SMillerDev commented May 10, 2023

xkia commented May 18, 2023

maxim-badarau-m10 commented May 18, 2023

JBYoshi commented May 24, 2023

JBYoshi commented Apr 3, 2023 •

edited

Loading

Bo98 commented May 10, 2023 •

edited

Loading