Change artifact behavior to moving instead of symlinking #13966

Merged
merged 9 commits into from May 31, 2016

Projects

None yet
@mwean
Contributor
mwean commented Sep 23, 2015

This is a first step towards change in installation behavior mentioned in #13201.
I had to skip the two_apps_correct tests because I don't know the desired functionality (e.g. should we copy the source to both targets and delete the source?)

@RomainLanz

Any news on this?

@mwean
Contributor
mwean commented Nov 25, 2015

I'm waiting for feedback from @jawshooah to make sure this is the correct approach.

@jawshooah jawshooah commented on an outdated diff Dec 10, 2015
lib/hbc/artifact/moved.rb
@@ -0,0 +1,142 @@
+class Hbc::Artifact::Moved < Hbc::Artifact::Base
+ ALT_NAME_ATTRIBUTE = 'com.apple.metadata:kMDItemAlternateNames'
+
+ attr_reader :source, :target
+
+ def english_name
+ self.class.artifact_english_name
+ end
+
+ def self.link_type_english_name
@jawshooah
jawshooah Dec 10, 2015 Member

This method can be removed, as it's not used anywhere.

@jawshooah jawshooah commented on an outdated diff Dec 10, 2015
lib/hbc/artifact/moved.rb
+ def summary
+ contents = @cask.artifacts[self.class.artifact_dsl_key].map do |artifact|
+ summarize_artifact(artifact)
+ end - [nil]
+
+ {
+ :english_description => "#{english_name}s managed by brew-cask:",
+ :contents => contents
+ }
+ end
+
+ def install_phase
+ each_artifact do |artifact|
+ load_specification(artifact)
+ next unless preflight_checks
+ add_altname_metadata
@jawshooah
jawshooah Dec 10, 2015 Member

I'm not sure how useful this is. Could you provide some justification for it?

@jawshooah
jawshooah Dec 15, 2015 Member

Didn't realize this was originally implemented in Hbc::Artifact::Symlinked and introduced in #3073 to address #2847. Judging from this comment, this shouldn't be necessary for actual files (i.e. not symlinks).

@jawshooah jawshooah commented on an outdated diff Dec 10, 2015
lib/hbc/artifact/moved.rb
+ def no_alternative_name?
+ source.basename.to_s.casecmp(target.basename) == 0
+ end
+
+ def build_altnames
+ altnames = existing_altname_metadata
+ odebug "Existing metadata is: '#{altnames}'"
+ altnames.concat(', ') if altnames.length > 0
+ altnames.concat(%Q{"#{target.basename}"})
+ "(#{altnames})"
+ end
+
+ def existing_altname_metadata
+ result = @command.run(
+ '/usr/bin/xattr',
+ :args => ['-p', ALT_NAME_ATTRIBUTE, target],
@jawshooah
jawshooah Dec 10, 2015 Member

Unless I'm mistaken, since this is called before move, there should be nothing at target. Where would the altnames come from?

@vitorgalvao
Member

@mwean Just to check, do you still intend on working on this?

@mwean
Contributor
mwean commented Jan 4, 2016

@vitorgalvao Yeah I'd definitely like to. I just moved, but I'll try to take a look this weekend.

@mwean
Contributor
mwean commented Feb 1, 2016

@vitorgalvao I rebased against master and made a couple more updates. I'm not sure what else needs to be done, and I don't know what the correct behavior should be for the two-app tests. I could maybe copy to both locations and delete the original? I'm not sure what the use case for that is. Let me know what you think.

@adidalal
Contributor
adidalal commented Feb 1, 2016

I agree that the two-app tests seem useless; at least, I've never seen it.
@jawshooah if you want to take a look at the PR when you're free?

@adidalal adidalal changed the title from DON'T MERGE - Change app artifact to move instead of link to [WIP] Change artifact behavior to moving instead of symlinking Feb 1, 2016
@jawshooah
Member

Will take another look at this tonight.

@ptb
Contributor
ptb commented Feb 9, 2016

I found one issue with this pull request, but unsure how to correctly correct it. If an app, Atom for example, has a binary that needs to be symlinked to /usr/local/bin, it complains that the symlink source isn't there. The workaround I've used is:

sed -i -e "s/@cask.staged_path/Hbc.appdir/" /usr/local/Library/Taps/caskroom/homebrew-cask/lib/hbc/artifact/symlinked.rb

but that breaks installs for fonts (at least). I'm not sure what the correct solution is, but figured I'd add that bit.

@mwean
Contributor
mwean commented Feb 9, 2016

@ptb are you saying that it tries to symlink the binary before it moves the app?

@ptb
Contributor
ptb commented Feb 9, 2016

No it tries to symlink after moving, but it expects that it still lives at the staged location (Caskroom).

brew cask install atom --force
==> Satisfying dependencies
complete
==> Downloading https://github.com/atom/atom/releases/download/v1.4.3/atom-mac.zip
######################################################################## 100.0%
==> Verifying checksum for Cask atom
==> Moving App 'Atom.app' to '/Users/admin/Applications/Atom.app'
Error: It seems the symlink source is not there: '/opt/homebrew-cask/Caskroom/atom/1.4.3/Atom.app/Contents/Resources/app/apm/node_modules/.bin/apm'
@vitorgalvao
Member

It had occurred to me recently this might be an issue. Basically this only happens if the binary to be linked is inside the app.

The solution would be to instead of having (to borrow atom as the example)

binary 'Atom.app/Contents/Resources/app/apm/node_modules/.bin/apm', target: 'apm'

have

binary "#{Hbc.appdir}/#{app[0]}/Contents/Resources/app/apm/node_modules/.bin/apm", target: 'apm'

Though that particular solution doesn’t work. All that’s needed is to add a new app_location (or something) special variable that we can employ in these cases, to just grab the install location.

@mwean
Contributor
mwean commented May 17, 2016

@ptb @vitorgalvao I made a failing spec for the embedded binary case, but I'm not sure how to proceed. I'm a little concerned putting interpolations into the path declaration like @vitorgalvao suggested because it would require Cask authors to know more about the cask inner workings. I wonder if we could match the beginning of the binary path against the app path (the "Atom.app" part in this example):

app 'Atom.app'
binary 'Atom.app/Contents/Resources/app/atom.sh', target: 'atom'

If there's a match, we'd know to look in the appdir. It would be a little ugly because we'd have to look at the app's artifacts from the symlink class, but it wouldn't require any change to the dsl. What do you think?

@vitorgalvao
Member
vitorgalvao commented May 17, 2016 edited

I feel like you’re trying to make the feature smart, and smart is what got us in this mess in the first place are we’re trying to avoid.

It is not a huge user-facing change. I’d much rather have it be explicit and work, even if it means contributors need to know something new, than having something smart that later on we find breaks on some cases.

It’s better to have it be dumb and make it smarter later, than the reverse.

@toonetown
Contributor

@mwean
it would require Cask authors to know more about the cask inner workings

I tend to agree with @vitorgalvao in that cask authors should be expected to know more about the cask inner workings. It should be simple and dumb and easy for end users to use and not get into trouble...but (IMO), it's OK to expect cask authors to need to know what they are doing.

It should be generally easy to create a cask...but it doesn't hurt to know the inner workings a bit in order to create one.

@mwean
Contributor
mwean commented May 17, 2016

Ok, that sounds good. What about having tokens instead of direct interpolation (like they do in Paperclip)? Something like

binary ":install_location/:app/Contents/Resources/app/apm/node_modules/.bin/apm"
@vitorgalvao
Member
vitorgalvao commented May 17, 2016 edited

Tokens seem clunky, to me. We use interpolation for some things, and symbols for others. They are well defined and predictable. Mixing here seems inappropriate.

Also, we really need a way to specify the index of what we mean. Some apps have multiple app stanzas, so we need to specify the one we’re pointing to. Something like binary "#{app_install_location}/#{app[0]}/…" seems acceptable, to me. We need to specify it’s the install location reserved for apps, in case we later need to do it for other artifacts.

@mwean
Contributor
mwean commented May 17, 2016

Ok, I actually wasn't clear what app[0] was referring to in your earlier comment. Could we do something more explicit like app_specifications[0].name? Something shorter would be better but I can't think of anything right now.

@vitorgalvao
Member
vitorgalvao commented May 17, 2016 edited

Could we do something more explicit like app_specifications[0].name?

Not sure I get why. It seems more confusing to me. app[0] would reduce complexity, as we’re talking about the literal value of the app stanza. To clarify, let’s use a made up cask:

app 'One.app'
app 'Two.app'
binary 'Two.app/Contents/Resources/app/script'

In this case, we’d use

app 'One.app'
app 'Two.app'
binary "#{app_install_location}/#{app[1]}/Contents/Resources/app/script"

I also wouldn’t oppose to just appdir instead of app_install_location.

To note that I’m not opposing app_specifications[0].name, I just don’t (yet?) see how it’s clearer than app[0], seeing as it directly refers to the actual stanza.

@mwean
Contributor
mwean commented May 17, 2016

When I saw app[0], I wasn't thinking of having multiple apps, I was thinking of args to the app method or something, since we destructure the args elsewhere (e.g. source_string, target_hash = artifact_spec). At any rate, if we're saying app is the collection of app stanzas, then why would app[0] necessarily return the app name? I'm totally fine either way, just want to make sure it's clear as possible.

@vitorgalvao
Member

Right. From a user/contributor-facing view, I was thinking of app as “a string that indicates what will be acted upon”.

Lets look at it another way, though. What would be easiest/would make more sense for you to implement?

@mwean
Contributor
mwean commented May 17, 2016

I'll try to spike on it a bit tonight and let you know.

@vitorgalvao
Member

Sounds good. Thank you.

@mwean
Contributor
mwean commented May 18, 2016

Ok, I added the appdir method to the DSL for interpolation. I decided to leave out the app[0] part because current casks (e.g. Atom) are already duplicating the app ("Atom.app") part, and it's much simpler to just continue with that. @ptb, this should now work with the embedded binaries. Let me know what you think.

@vitorgalvao
Member

current casks (e.g. Atom) are already duplicating the app ("Atom.app") part, and it's much simpler to just continue with that.

Fair enough.

@ptb, this should now work with the embedded binaries.

But you’ll still need to update the casks by hand first, to try it out.

@caskroom/maintainers We should test this for a bit and see if we find any issue while using.

@adidalal
Contributor

What's the best way to test locally?

@mwean
Contributor
mwean commented May 18, 2016

@vitorgalvao should I work on a script or something to update all the existing casks with apps and binaries? That seems like it would take some work.

@vitorgalvao
Member
vitorgalvao commented May 19, 2016 edited

Best way to test locally:

git clone https://github.com/mwean/homebrew-cask.git # clone @mwean’s repo with this change
./homebrew-cask/developer/bin/develop_brew_cask # make his repo the main one to use

As develop_brew_cask warns, do not run brew update while in this mode. To get back to regular usage, run production_brew_cask in that same directory.

@mwean Just tested on a clean VM, and noted something. This is still using ~/Applications by default. With this change, /Applications should be the default. With that, only point 5 of the “The end of linking, and the beginning of moving (technical)” section in #13201 will remain of the first part of this change, but that is not mandatory to merge this particular PR.

As for the script, you may work on it if you want, but if not, I’d be happy to do it. It’s likely many of the casks will have to be manually altered anyway, and that’s a boring job that I’m prepared to do.

@jawshooah
Member

Note that, if you already have a local clone of the repo, you can do the following from its root:

brew install hub
hub checkout https://github.com/caskroom/homebrew-cask/pull/13966
./developer/bin/develop_brew_cask
@mwean
Contributor
mwean commented May 19, 2016

@vitorgalvao this only handles app stanzas now. Should I switch all the other artifacts (except binary) to use move?

I'll update the default appdir.

I'll try looking at writing a migration script this weekend, but feel free to tackle it if you have time before then.

@vitorgalvao
Member

Should I switch all the other artifacts (except binary) to use move?

Yes, might as well switch them all from the start.

@mwean
Contributor
mwean commented May 20, 2016

I forgot that I skipped the two-app specs because I didn't know how to handle that case. We could copy the app and then do some sort of cleanup at the end of the cask installation or something. I don't really understand the use-case anyway.

@MikeMcQuaid
Contributor

@vitorgalvao Something we've found useful for testing relatively invasive changes like this is guarding the new behaviour behind an environment variable (e.g. CASK_MOVE_APPS), disabling it by default and then allowing people to enable it gradually. That also means you can merge PRs before needing to ship it to people so review becomes easier.

@claui
Member
claui commented May 20, 2016 edited

I ❤️ this PR and the way it is implemented!

@mwean How are we going to handle brew cask install --force?
With plain brew cask install, I still see the warning:

Add the "--force" option to force re-install.

… which sets up my expectation that adding --force would trash and replace the existing app (if and only if the app is managed by brew-cask) no matter what.

What I’m observing instead is that brew cask install --force now refuses to overwrite the existing app. I suggest this to be changed to avoid confusion and to give the --force option a chance to continue making sense.

Any thoughts?

Edit: Removed unnecessary intelligence.

@mwean
Contributor
mwean commented May 20, 2016

@claui that's a good point. I was also thinking about that when I was looking at the specs that prevent clobbering existing artifacts. I was thinking that when we handle updates, it would be hard to be sure that we're overwriting the app that we installed and not one the user subsequently installed manually. Personally, I'd be happy to have it always overwrite, but a maintainer should weigh in on the desired behavior.

@adidalal
Contributor

Agreed with the always overwrite - it shouldn't matter where the app came from, as desired user behavior should result in the latest (version in Cask) should be installed....

but in fact, this is where auto_updates true will be of use, as maybe that should be the only reason to not overwrite the existing app (if I'm remembering the original proposal correctly). We can handle misbehaving/auto-updating Casks on a manual basis after the initial merge, as adding that line isn't particularly onerous (and even probably scriptable, to be honest)

@claui claui added a commit to claui/homebrew-cask that referenced this pull request May 21, 2016
@claui claui sage.rb: Switch mirror; update to v7.2; fix URLs
The latest stable version of SageMath is [7.2](http://mirrors.mit.edu/sage/osx/intel/index.html) and can be found on the MIT mirror.

Currently, the El Capitan branch of this cask is broken because it points to [go-parts.com](http://mirrors-usa.go-parts.com/sage/sagemath/osx/intel/), a mirror which seems not to carry neither the stable 7.1 version nor the current stable v7.2.

The site is also not part of the [list of download servers on sagemath.org](http://www.sagemath.org/download-mac.html). Because of this, I switched the El Capitan branch to point to mirrors.mit.edu/sage just like the other branches do.

This commit also updates broken URLs for the Mavericks and Yosemite branches.

http://mirrors.mit.edu/sage/osx/intel/sage-7.2-OSX_10.11.4-x86_64.tar.bz2

This cask also happens to symlink a binary which lives inside the moved artifact. So this is one of the cases where we might want to use the new `appdir` method that is being introduced in #13966.
f1a7be1
@claui claui added a commit to claui/homebrew-cask that referenced this pull request May 21, 2016
@claui claui sage.rb: Switch mirror; update to v7.2; fix URLs
The latest stable version of SageMath is [7.2](http://mirrors.mit.edu/sage/osx/intel/index.html) and can be found on the MIT mirror.

Currently, the El Capitan branch of this cask is broken because it points to [go-parts.com](http://mirrors-usa.go-parts.com/sage/sagemath/osx/intel/), a mirror which seems not to carry neither the stable 7.1 version nor the current stable v7.2.

The site is also not part of the [list of download servers on sagemath.org](http://www.sagemath.org/download-mac.html). Because of this, I switched the El Capitan branch to point to mirrors.mit.edu/sage just like the other branches do.

This commit also updates broken URLs for the Mavericks and Yosemite branches.

http://mirrors.mit.edu/sage/osx/intel/sage-7.2-OSX_10.11.4-x86_64.tar.bz2

This cask also happens to symlink a binary which lives inside the moved artifact. So this is one of the cases where we might want to use the new `appdir` method that is being introduced in #13966.
c7919fa
@claui claui added a commit to claui/homebrew-cask that referenced this pull request May 21, 2016
@claui claui sage.rb: Switch mirror; update to v7.2; fix URLs
The latest stable version of SageMath is [7.2](http://mirrors.mit.edu/sage/osx/intel/index.html) and can be found on the MIT mirror.

Currently, the El Capitan branch of this cask is broken because it points to [go-parts.com](http://mirrors-usa.go-parts.com/sage/sagemath/osx/intel/), a mirror which seems not to carry neither the stable 7.1 version nor the current stable v7.2.

The site is also not part of the [list of download servers on sagemath.org](http://www.sagemath.org/download-mac.html). Because of this, I switched the El Capitan branch to point to mirrors.mit.edu/sage just like the other branches do.

This commit also updates broken URLs for the Mavericks and Yosemite branches.

http://mirrors.mit.edu/sage/osx/intel/sage-7.2-OSX_10.11.4-x86_64.tar.bz2

This cask also happens to symlink a binary which lives inside the moved artifact. So this is one of the cases where we might want to use the new `appdir` method that is being introduced in #13966.
051a365
@claui claui added a commit to claui/homebrew-cask that referenced this pull request May 21, 2016
@claui claui sage.rb: Switch mirror; update to v7.2; fix URLs
The latest stable version of SageMath is [7.2](http://mirrors.mit.edu/sage/osx/intel/index.html) and can be found on the MIT mirror.

Currently, the El Capitan branch of this cask is broken because it points to [go-parts.com](http://mirrors-usa.go-parts.com/sage/sagemath/osx/intel/), a mirror which seems not to carry neither the stable 7.1 version nor the current stable v7.2.

The site is also not part of the [list of download servers on sagemath.org](http://www.sagemath.org/download-mac.html). Because of this, I switched the El Capitan branch to point to mirrors.mit.edu/sage just like the other branches do.

This commit also updates broken URLs for the Mavericks and Yosemite branches.

http://mirrors.mit.edu/sage/osx/intel/sage-7.2-OSX_10.11.4-x86_64.tar.bz2

This cask also happens to symlink a binary which lives inside the moved artifact. So this is one of the cases where we might want to use the new `appdir` method that is being introduced in #13966.
714f515
@vitorgalvao vitorgalvao added a commit that referenced this pull request May 21, 2016
@claui @vitorgalvao claui + vitorgalvao sage.rb: Switch mirror; update to v7.2; fix URLs (#21353)
The latest stable version of SageMath is [7.2](http://mirrors.mit.edu/sage/osx/intel/index.html) and can be found on the MIT mirror.

Currently, the El Capitan branch of this cask is broken because it points to [go-parts.com](http://mirrors-usa.go-parts.com/sage/sagemath/osx/intel/), a mirror which seems not to carry neither the stable 7.1 version nor the current stable v7.2.

The site is also not part of the [list of download servers on sagemath.org](http://www.sagemath.org/download-mac.html). Because of this, I switched the El Capitan branch to point to mirrors.mit.edu/sage just like the other branches do.

This commit also updates broken URLs for the Mavericks and Yosemite branches.

http://mirrors.mit.edu/sage/osx/intel/sage-7.2-OSX_10.11.4-x86_64.tar.bz2

This cask also happens to symlink a binary which lives inside the moved artifact. So this is one of the cases where we might want to use the new `appdir` method that is being introduced in #13966.
54ee18c
@vitorgalvao
Member

@mwean The use case is we have some casks that really do have two app stanzas. Some packages really consist of multiple app bundles.

Regarding auto_updates, the rule is: when we have upgrade functionality, every installed cask that has auto_updates (even if the cask was updated) will be ignored by us (and we’ll let the app take care of the update).

As for overwriting, I think it’s good we do not overwrite by default, but do overwrite when using --force. The reason is simple: it’s not at all uncommon for different casks to have artifacts with the same name (see all eclipse casks). Overwriting those by default could be a nasty surprise.

@MikeMcQuaid That is a great idea, and we should adopt that as well. For this particular case, however, I’m fine with letting the breaking change come in. Reason being we’re so low on core developers, this important feature is being contributed by someone outside the main team, and I really do not want to take @mwean’s time more than necessary. We’ve also warned everyone things would break and the response has been quite positive (people really want this new behaviour), so I’m fine with it.

@mwean
Contributor
mwean commented May 22, 2016

The use case is we have some casks that really do have two app stanzas. Some packages really consist of multiple app bundles.

That makes sense, but the spec is specifically for the same app going to two different locations:

# test/support/Casks/with-two-apps-correct.rb
app 'Caffeine.app'
app 'Caffeine.app', target: 'AnotherName.app'

Having two app stanzas is fine, as long as they don't use the same source. Do you still want to support the use case in this spec?

As for overwriting, I think it’s good we do not overwrite by default, but do overwrite when using --force.

Sounds good. This should work correctly with @claui's branch.

@claui
Member
claui commented May 22, 2016

@mwean I have checked every cask with two or more app stanzas:

grep -c '^\s*app\s' Casks/*.rb | ruby -ne 'puts $1 if ~/(.*):[^01]/' | xargs mate

and found that the scenario from the spec never comes up even once. I would suggest that that particular spec be abandoned because it has no redeeming value.

@claui
Member
claui commented May 22, 2016

This should work correctly with @claui's branch.

Speaking of working correctly, I’ve been bitten earlier today with one more issue: brew cask install lingo --force finds an existing target, tries to trash it but fails because I happen not to be the owner.

Why not? Turns out I had bought the same app from the MAS years ago but forgot about it. Today I had my head in the clouds as always so I ran install lingo --force on my branch. The conflicting Lingo.app, being a MAS app, belonged to root so it didn’t budge. I feel it’s pretty safe to assume that in general, end users will not remember whether their app came from the MAS or HBC.

So I’m left wondering what HBC should do in that case?

  1. Leave the artifact staged and display the Not moved message?
  2. Go sudo, asking the user for the password?
  3. Give up and ask the user to manually resolve the conflict?

@mwean @vitorgalvao Any thoughts as to what would be the most reasonable course of action here?

@vitorgalvao
Member

@mwean Yes, the way that spec is presented is a bit weird. There are indeed some casks with more than one app, but no, never the same, as @claui pointed out.

All that said, I agree with your previous comment, lets leave out the whole app[0] nonsense. It works well as is.

@claui Your second scenario seems the most reasonable. If we’re saying --force, then forceful it should be, consequences be damned (presumably, the user understands that force is the “do whatever it takes to make this work” option).

That said, I have no strong commitment to that behaviour. If anyone thinks one of the others should be the case and makes a good case for it, I’m fine either way.

@claui claui added a commit to claui/homebrew-cask that referenced this pull request May 22, 2016
@claui claui Forcibly overwrite artifacts, modifying flags and using `sudo` if needed
- This commit implements [the proposed behavior for `install --force`](caskroom#13966 (comment)) when a target already exists and has either permission problems or is not owned by the user.

- The changes apply only when the `force` option is given.

- As a safety net, t

- The two existing blacklists `SYSTEM_DIRS` and `UNDELETABLE_DIRS` have been consolidated into the `Hbc::MacOS` module.

- `UNDELETABLE_DIRS` now also contains all the entries from `SYSTEM_DIRS` which was a to-do anyway.

- The two blacklists are now also frozen for good measure.

- The utility method `permissions_rmtree` was moved to `Hbc::Utils`.

- The `tried_permissions` part in `Utils` now falls back correctly when there are also ownership issues at the same time.

- Introduced a separate `current_user` method for mocking.

- Added an optional feature to `FakeSystemCommand` so it can now act as a proxy to `SystemCommand`.

- Added tests for various `permissions_rmtree` cases.
12369d5
@claui claui added a commit to claui/homebrew-cask that referenced this pull request May 22, 2016
@claui claui Forcibly overwrite artifacts, modifying flags and using `sudo` if needed
- This commit implements [the proposed behavior for `install --force`](caskroom#13966 (comment)) when a target already exists and has either permission problems or is not owned by the user.

- The changes apply only when the `force` option is given.

- Reused the existing safeguard from the `.pkg` artifact to prevent deleting important directories by bug or mistake

- The two existing blacklists `SYSTEM_DIRS` and `UNDELETABLE_DIRS` have been consolidated into the `Hbc::MacOS` module.

- `UNDELETABLE_DIRS` now also contains all the entries from `SYSTEM_DIRS` which was a to-do anyway.

- The two blacklists are now also frozen for good measure.

- The utility method `permissions_rmtree` was moved to `Hbc::Utils`.

- The `tried_permissions` part in `Utils` now falls back correctly when there are also ownership issues at the same time.

- Introduced a separate `current_user` method for mocking.

- Added an optional feature to `FakeSystemCommand` so it can now act as a proxy to `SystemCommand`.

- Added tests for various `permissions_rmtree` cases.
f470507
@mwean
Contributor
mwean commented May 22, 2016

Great, I'll update the 2-app specs and merge in @claui's branch.

@mwean
Contributor
mwean commented May 23, 2016

@vitorgalvao @claui: Ok, I fixed the 2-app spec, updated all the binary stanzas, and merged in @claui's branch. I'm not sure what the failure is on Travis, but I think we might be good to go with this!

@adidalal
Contributor
adidalal commented May 23, 2016 edited

@mwean Ignore the Travis error - it's been nearly impossible to pin down, but it's in no way indicative of an actual failure/error in the PR.

Will let one of the other maintainers have a look, but LGTM.

Easiest way to "migrate" might just be to get a list of all casks, uninstall them and wipe out /Caskroom and associated symlinks, and then reinstall after this change is merged, but open to better methods - I wouldn't be opposed to tagging a new release as well, just as a symbolic measure.

@muescha
Contributor
muescha commented May 24, 2016

i would prefer moving apps out of /Caskroom is better than reinstalling app (maybe someone dont like a new version installed)

@claui
Member
claui commented May 24, 2016

@adidalal Are you thinking of a scripted migration? Would such a migration be optional or rather mandatory?

@adidalal
Contributor
adidalal commented May 24, 2016 edited

No opinion either way - with such a large change, there probably isn't a "best" way to proceed, but open to ideas for how to make it a not-terrible experience. If it's getting off-topic here, this discussion can be moved to a new issue, but it's one that should happen before this change is merged and made final

EDIT: See below

@vitorgalvao
Member

@muescha @claui @adidalal No migration. That’s the point made at the end of #13201.

Doesn’t this mean a bunch of stuff might break?

(…) transition strategies is an inglorious job (everything will be discarded in the end, after all) that puts on hold even the smallest of changes indefinitely. It is incredibly time consuming, has variable results (we still get a bunch of issues pertaining to it), and (above all) kills momentum. Furthermore, this is a big change and there’s really no good way to make this seamlessly.

If anyone wants to devise a transition strategy, that is fine. We’d need commitment to that, however, meaning the transitioner will be responsible for implementation, bugs pertaining to it, and removing the code in a few months when everyone is presumably transitioned.

#13201 made clear from the start we’re willing to break things with this change. All things considered, it’s not even a huge break, your old installs won’t stop working. I’ll greatly advise anyone against a transition strategy for this. Even brew cask list is broken and we don’t keep track of installed casks correctly. There’s no way you’ll write a good transition when everything else is falling around you and you can’t be sure you can lean on other parts of the codebase.

Lets leave it be. Accept the breakage as we already knew it would happen, and move on.

Leaving this open for a few more days for disagreements, but I’ll say I’m excited to have this merged in. Not everything in #13201 is present, but the most important bits are, and it’s another step to the finally right direction.

@claui claui added a commit to claui/homebrew-cask that referenced this pull request May 24, 2016
@claui claui Fix triumph.rb: Remove DDP Player.app artifact
Triumph.app contains in its bundle three helper tools. Those tools are meant to be accessed from the menu bar while using Triumph.

One of those tools, DDP Player.app, is defined as an artifact though. With upcoming PR #13966, `cask install` would rip out DDP Player.app from the bundle. As a consequence, invoking the *DDP Player* menu item would cause Triumph to crash.

This commit fixes the issue by removing the artifact altogether. All the tools (including DDP Player) should be launched from the *Tools* menu as intended, e. g.:

> Triumph » Tools » DDP Player
bbb7ace
@claui claui referenced this pull request May 24, 2016
Merged

Fix triumph.rb: Remove DDP Player.app artifact #21413

3 of 3 tasks complete
@vitorgalvao vitorgalvao added a commit that referenced this pull request May 24, 2016
@claui @vitorgalvao claui + vitorgalvao Fix triumph.rb: Remove DDP Player.app artifact (#21413)
Triumph.app contains in its bundle three helper tools. Those tools are meant to be accessed from the menu bar while using Triumph.

One of those tools, DDP Player.app, is defined as an artifact though. With upcoming PR #13966, `cask install` would rip out DDP Player.app from the bundle. As a consequence, invoking the *DDP Player* menu item would cause Triumph to crash.

This commit fixes the issue by removing the artifact altogether. All the tools (including DDP Player) should be launched from the *Tools* menu as intended, e. g.:

> Triumph » Tools » DDP Player
6e13433
@ptb
Contributor
ptb commented May 25, 2016

I believe the changes to all of the casks to make "#{appdir}" work correctly will need to be enclosed in double quotes, otherwise these are the types of errors that will be produced:

Error: It seems the symlink source is not there: '/usr/local/Caskroom/atom/1.7.3/#{appdir}/Atom.app/Contents/Resources/app/apm/node_modules/.bin/apm

@mwean
Contributor
mwean commented May 25, 2016

Great catch @ptb, I'll get that fixed

@mwean mwean added a commit to mwean/homebrew-cask that referenced this pull request May 25, 2016
@claui @mwean claui + mwean Forcibly overwrite artifacts, modifying flags and using `sudo` if needed
- This commit implements [the proposed behavior for `install --force`](caskroom#13966 (comment)) when a target already exists and has either permission problems or is not owned by the user.

- The changes apply only when the `force` option is given.

- Reused the existing safeguard from the `.pkg` artifact to prevent deleting important directories by bug or mistake

- The two existing blacklists `SYSTEM_DIRS` and `UNDELETABLE_DIRS` have been consolidated into the `Hbc::MacOS` module.

- `UNDELETABLE_DIRS` now also contains all the entries from `SYSTEM_DIRS` which was a to-do anyway.

- The two blacklists are now also frozen for good measure.

- The utility method `permissions_rmtree` was moved to `Hbc::Utils`.

- The `tried_permissions` part in `Utils` now falls back correctly when there are also ownership issues at the same time.

- Introduced a separate `current_user` method for mocking.

- Added an optional feature to `FakeSystemCommand` so it can now act as a proxy to `SystemCommand`.

- Added tests for various `permissions_rmtree` cases.
423eb98
@mwean
Contributor
mwean commented May 25, 2016

Ok, should be fixed now

@ptb
Contributor
ptb commented May 25, 2016

The casks that specify a target for the binary didn't get changed (e.g. Atom, Github Desktop, others).
(And I'll say thank you now, for all of your work on this!)

@jawshooah jawshooah commented on an outdated diff May 25, 2016
Casks/atom.rb
@@ -14,8 +14,8 @@
depends_on macos: '>= :mountain_lion'
app 'Atom.app'
- binary 'Atom.app/Contents/Resources/app/apm/node_modules/.bin/apm', target: 'apm'
- binary 'Atom.app/Contents/Resources/app/atom.sh', target: 'atom'
+ binary '#{appdir}/Atom.app/Contents/Resources/app/apm/node_modules/.bin/apm', target: 'apm'
@jawshooah
jawshooah May 25, 2016 Member

Still using single-quotes here, and in other casks.

@mwean
Contributor
mwean commented May 25, 2016

Ah, gotta fix my regex...

@mwean mwean added a commit to mwean/homebrew-cask that referenced this pull request May 25, 2016
@claui @mwean claui + mwean Forcibly overwrite artifacts, modifying flags and using `sudo` if needed
- This commit implements [the proposed behavior for `install --force`](caskroom#13966 (comment)) when a target already exists and has either permission problems or is not owned by the user.

- The changes apply only when the `force` option is given.

- Reused the existing safeguard from the `.pkg` artifact to prevent deleting important directories by bug or mistake

- The two existing blacklists `SYSTEM_DIRS` and `UNDELETABLE_DIRS` have been consolidated into the `Hbc::MacOS` module.

- `UNDELETABLE_DIRS` now also contains all the entries from `SYSTEM_DIRS` which was a to-do anyway.

- The two blacklists are now also frozen for good measure.

- The utility method `permissions_rmtree` was moved to `Hbc::Utils`.

- The `tried_permissions` part in `Utils` now falls back correctly when there are also ownership issues at the same time.

- Introduced a separate `current_user` method for mocking.

- Added an optional feature to `FakeSystemCommand` so it can now act as a proxy to `SystemCommand`.

- Added tests for various `permissions_rmtree` cases.
9e32b6d
@mwean
Contributor
mwean commented May 25, 2016

Ok, let's try that again

@jawshooah
Member

Need to change dest_path to Hbc.appdir here as well.

@mwean mwean added a commit to mwean/homebrew-cask that referenced this pull request May 26, 2016
@claui @mwean claui + mwean Forcibly overwrite artifacts, modifying flags and using `sudo` if needed
- This commit implements [the proposed behavior for `install --force`](caskroom#13966 (comment)) when a target already exists and has either permission problems or is not owned by the user.

- The changes apply only when the `force` option is given.

- Reused the existing safeguard from the `.pkg` artifact to prevent deleting important directories by bug or mistake

- The two existing blacklists `SYSTEM_DIRS` and `UNDELETABLE_DIRS` have been consolidated into the `Hbc::MacOS` module.

- `UNDELETABLE_DIRS` now also contains all the entries from `SYSTEM_DIRS` which was a to-do anyway.

- The two blacklists are now also frozen for good measure.

- The utility method `permissions_rmtree` was moved to `Hbc::Utils`.

- The `tried_permissions` part in `Utils` now falls back correctly when there are also ownership issues at the same time.

- Introduced a separate `current_user` method for mocking.

- Added an optional feature to `FakeSystemCommand` so it can now act as a proxy to `SystemCommand`.

- Added tests for various `permissions_rmtree` cases.
1adba61
@mwean
Contributor
mwean commented May 27, 2016

I'm trying to reproduce the failure on CI, but I'm getting Hbc::CaskError: Uh oh, could not figure out how to unpack '/var/folders/81/krskm0797s78hqmfdx7v4_k40000gn/T/homebrew_tests20160527-29970-m1ln87/cache/stuffit-container-1.2.3.sit'. Could someone help me with this?

@jawshooah
Member

@mwean Do you have unar installed?

Separate but related issue: that test uses a test cask containing an artifact stanza with no target, which should never happen. Will fix in a bit.

@mwean
Contributor
mwean commented May 29, 2016 edited

Haha I was reading that as unrar... Problem solved.

@jawshooah I added target: sheldonmac/v1.0 and it's working now, thanks!

@jawshooah
Member

@mwean There are some conflicts now that #21549 has been merged in. Resolve those and push, and I'll be glad to merge this once Travis goes green.

@jawshooah
Member

@caskroom/maintainers
Whaaaaaaaaat

mwean and others added some commits Sep 23, 2015
@mwean mwean Change app artifact to move instead of link
First step towards change in installation behavior mentioned in [13201]
724ca0f
@mwean mwean Fix handling of binaries linked from inside of app bundles
Also adds `appdir` method for interpolation in stanzas
d16e4f9
@mwean mwean Change appdir to root Applications directory 25c2b01
@mwean mwean Update 2-app tests 246e749
@claui @mwean claui Refactor: add options, ivars to `Installer`, `Download`
In preparation for upcoming changes, this commit cleans up some code. The commit includes:

- In order to reduce unnecessary object passing, make both the `force` and `skip_cask_deps` option into instance variables of the `Installer` class

- Introduce options hashes to initializers of both the `Installer` and `Download` class

- When the `install --force` command enters the fetch phase, make it explicit in the code that fetching is never enforced in that case.

- Update tests
9876d17
@claui @mwean claui Force overwrite artifacts on `--force` reinstall
This commit changes the behavior of a `Moved` artifact such that if the target already exists, `brew cask install --force` will remove the existing target before moving the staged artifact.

In that case, the warning message will say *overwriting* instead of *not moving*.

The behavior of plain `brew cask install` remains unchanged; the same goes for the warning message for that case.
6500fa1
@mwean mwean Change remaining artifacts to move instead of symlink ccc3c25
@mwean mwean Update casks to use appdir in binary paths 91f19b1
@claui @mwean claui Forcibly overwrite artifacts, modifying flags and using `sudo` if needed
- This commit implements [the proposed behavior for `install --force`](caskroom#13966 (comment)) when a target already exists and has either permission problems or is not owned by the user.

- The changes apply only when the `force` option is given.

- Reused the existing safeguard from the `.pkg` artifact to prevent deleting important directories by bug or mistake

- The two existing blacklists `SYSTEM_DIRS` and `UNDELETABLE_DIRS` have been consolidated into the `Hbc::MacOS` module.

- `UNDELETABLE_DIRS` now also contains all the entries from `SYSTEM_DIRS` which was a to-do anyway.

- The two blacklists are now also frozen for good measure.

- The utility method `permissions_rmtree` was moved to `Hbc::Utils`.

- The `tried_permissions` part in `Utils` now falls back correctly when there are also ownership issues at the same time.

- Introduced a separate `current_user` method for mocking.

- Added an optional feature to `FakeSystemCommand` so it can now act as a proxy to `SystemCommand`.

- Added tests for various `permissions_rmtree` cases.
9664b9f
@mwean
Contributor
mwean commented May 31, 2016

@jawshooah I think you're good to go!

@jawshooah jawshooah merged commit 4738316 into caskroom:master May 31, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jawshooah jawshooah changed the title from [WIP] Change artifact behavior to moving instead of symlinking to Change artifact behavior to moving instead of symlinking May 31, 2016
@ptb ptb referenced this pull request in caskroom/homebrew-versions May 31, 2016
Merged

Changes to support caskroom/homebrew-cask#13966 #2138

@vitorgalvao vitorgalvao added a commit to caskroom/homebrew-versions that referenced this pull request May 31, 2016
@ptb @vitorgalvao ptb + vitorgalvao Changes to support caskroom/homebrew-cask#13966 (#2138) 2a10dde
@elyscape elyscape added a commit to elyscape/dotfiles that referenced this pull request May 31, 2016
@elyscape elyscape Put Caskroom in /usr/local
Additionally, .apps are installed to /Applications instead of
~/Applications by default as of caskroom/homebrew-cask#13966, so setting
appdir is no longer necessary.
92f4b92
@elyscape elyscape added a commit to elyscape/dotfiles that referenced this pull request May 31, 2016
@elyscape elyscape Put Caskroom in /usr/local
Additionally, .apps are installed to /Applications instead of
~/Applications by default as of caskroom/homebrew-cask#13966, so setting
appdir is no longer necessary.
23679c1
@claui
Member
claui commented May 31, 2016

Yay!!! What a fantastic day for Homebrew Cask! <3

@mwean
Contributor
mwean commented May 31, 2016

So what's the next step @vitorgalvao??

@vitorgalvao
Member
vitorgalvao commented May 31, 2016 edited

Next steps:

  • Get rid of versioned subdirectories in the Caskroom. The .metadata directory will still need to exist (although its innards won’t have the versioned app directory either) since it will be useful to save a snapshot of the installed cask to handle updates/uninstalls at a future time.
  • Implement :required_location (apps that will benefit from it at the start are mentioned in #12858).
    • And with it, --force-appdir (these two really go hand in hand).

And that is actually way less of #13201 left than I realised!

We still have a lot to do after that, but closing #13201 will be a huge milestone (this merged pull request already is!), and it makes me really glad to see the project completing this step, one further in the right direction.

Thank you greatly, @mwean, @claui, @jawshooah, and many others (too many to mention all without missing some and making this sound like an awards ceremony speech) for all the work.

@muescha muescha commented on the diff Jun 1, 2016
```
Note that you still can override the environment variable `HOMEBREW_CASK_OPTS` by _explicitly_ providing options in the command line:
```bash
-# Will force the Chrome app to be linked to ~/Applications
-# even though HOMEBREW_CASK_OPTS specified /Applications
-$ brew cask install --appdir="~/Applications" google-chrome
+# Will force the Chrome app to be linked to /Applications
@muescha
muescha Jun 1, 2016 edited Contributor

linked -> moved

@muescha muescha commented on the diff Jun 1, 2016
doc/cask_language_reference/all_stanzas.md
@@ -33,7 +33,7 @@ Each Cask must declare one or more *artifacts* (i.e. something to install).
| `audio_unit_plugin`| yes | relative path to an Audio Unit plugin that should be linked into the `~/Library/Audio/Components` folder on installation
| `vst_plugin` | yes | relative path to a VST Plugin that should be linked into the `~/Library/Audio/VST` folder on installation
| `vst3_plugin` | yes | relative path to a VST3 Plugin that should be linked into the `~/Library/Audio/VST3` folder on installation
-| `suite` | yes | relative path to a containing directory that should be linked into the `~/Applications` folder on installation (see also [Suite Stanza Details](stanzas/suite.md))
+| `suite` | yes | relative path to a containing directory that should be linked into the `/Applications` folder on installation (see also [Suite Stanza Details](stanzas/suite.md))
@muescha
muescha Jun 1, 2016 edited Contributor

linked --> moved

@muescha muescha commented on the diff Jun 1, 2016
man/man1/brew-cask.1
@@ -149,7 +149,7 @@ Give additional feedback during installation\.
.
.TP
\fB\-\-appdir=<path>\fR
-Target location for Application links\. The default value is \fB~/Applications\fR\.
+Target location for Application links\. The default value is \fB/Applications\fR\.
@muescha
muescha Jun 1, 2016 Contributor

it is not a link anymore

@muescha muescha commented on the diff Jun 1, 2016
@@ -176,35 +176,34 @@ You can also modify the default installation locations used when issuing `brew c
* `--caskroom=/my/path` determines where the actual applications will be located.
Should be handled with care — setting it outside `/opt` or your home directory might mess up your system.
Default is `/opt/homebrew-cask/Caskroom`
@muescha
muescha Jun 1, 2016 Contributor

confused with caskroom and appdir -> we still need caskroom?

@vitorgalvao
vitorgalvao Jun 1, 2016 Member

Yes, we still need to Caskroom. See #13201, specifically point 4 of this screenshot:
image

@muescha
muescha Jun 1, 2016 Contributor

(#13201 is closed for comments - so i answer here)

so it is a "cache" where are placed "leftovers" from installation? i expected that hbc creates only temp dirs and then deleted all other stuff.

it should be better described in README/USAGE and whats the difference.

@vitorgalvao
vitorgalvao Jun 1, 2016 edited Member

i expected that hbc creates only temp dirs and then deleted all other stuff

That isn’t temporary stuff, though. Caskroom will keep three things (the first one is not mentioned in the screenshot):

  • A .metadata directory that contains a snapshot of the state of the cask in the moment you installed it (this will be useful in the future to correctly deal with uninstalls).
  • Any CLI artifacts that will be linked to /usr/local/bin/ (or whatever). Since they are links, they need a place to be where they are linked from.
  • Uninstall scripts and other things that come in the dmg (or whatever) that although not integral to the app, might be relevant t to some users.
@muescha
muescha Jun 1, 2016 Contributor

thx for details :)

this info should be added to the README/USAGE

@vitorgalvao
vitorgalvao Jun 1, 2016 Member

Since the google-chrome staged at '/opt/homebrew-cask/Caskroom/google-chrome/latest' message is already output, a FAQ document might be more appropriate, for inquiring users (I’ll wager most won’t care, so having this in such a prominent place like the README stops making sense).

@muescha
muescha Jun 1, 2016 Contributor

ah - ok - i dont see the line google-chrome staged at ... before. it makes is more clear

but since its moved:

if an artefact has only one binary/app then this staged dir in Caskroom leaves empty (i assume the .metadata is per app but not always visible)?

@toonetown
toonetown Jun 1, 2016 edited Contributor

Are there plans to perhaps move the caskroom directory under $(brew --prefix) at some point now that artifacts are moved instead of linked? I think that the original reason for putting it in /opt is that spotlight would index there - but it wouldn't index in /usr/local. See #2494. Now that artifacts are actually moved, it wouldn't hurt (IMO) to put the Caskroom by default at $(brew --prefix)/Caskroom.

@vitorgalvao
vitorgalvao Jun 1, 2016 edited Member

@muescha Yes, the directory may look empty (it will have .metadata), and the staged at message will even say 0B. Not a huge issue to have it there, though.

@toonetown Yes, /opt was originally chosen for that reason, but later on it proved to be the right directory anyway.

All that said, since we’re more integrated with homebrew and that integration will only become more prevalent, I’d have no quarrel in moving under $(brew --prefix). It will even save us from having to ask for a password on first install.

Mind opening a new issue so we can discuss it?

@muescha muescha commented on the diff Jun 1, 2016
README.md
@@ -30,7 +30,7 @@ $ brew cask install google-chrome
And there we have it. Google Chrome installed with a few quick commands: no clicking, no dragging, no dropping.
@muescha
muescha Jun 1, 2016 Contributor

code snippet for brew cask install google-chrome should be changed from Symlinking to Moving (line 23 of collapsed code in diff)

@vitorgalvao
vitorgalvao Jun 1, 2016 Member

This one I have already fixed yesterday.

@muescha muescha commented on the diff Jun 1, 2016
@@ -53,7 +53,7 @@ Easy peasy:
$ brew cask uninstall google-chrome
```
@muescha
muescha Jun 1, 2016 Contributor

code snippet for brew cask install google-chrome should be changed from Symlinking to Moving (line 44 of collapsed code in diff)

@vitorgalvao
vitorgalvao Jun 1, 2016 Member

This one I have already fixed yesterday as well.

@mwean
Contributor
mwean commented Jun 1, 2016

@muescha I think there was a bad rebase that reverted my doc changes. I'll make a new PR to fix them

@arbourd arbourd added a commit to arbourd/dotfiles that referenced this pull request Jun 1, 2016
@arbourd arbourd Remove instruction to mv casks
No longer necessary as of:
caskroom/homebrew-cask#13966
3ccbed0
@vitorgalvao
Member

Fixing issues caught by @muescha in #21597.

@toonetown
Contributor

I just have to say, it might be completely in my head (or due to the fact that I uninstalled/reinstalled all my casks to get the "move" behavior), but my applications seem to launch a LOT faster and spotlight seems to be a LOT more responsive since this change has been implemented. Nice work!!!

@chiselwright chiselwright added a commit to chiselwright/homebrew-cask that referenced this pull request Jun 15, 2016
@claui @chiselwright claui + chiselwright sage.rb: Switch mirror; update to v7.2; fix URLs (#21353)
The latest stable version of SageMath is [7.2](http://mirrors.mit.edu/sage/osx/intel/index.html) and can be found on the MIT mirror.

Currently, the El Capitan branch of this cask is broken because it points to [go-parts.com](http://mirrors-usa.go-parts.com/sage/sagemath/osx/intel/), a mirror which seems not to carry neither the stable 7.1 version nor the current stable v7.2.

The site is also not part of the [list of download servers on sagemath.org](http://www.sagemath.org/download-mac.html). Because of this, I switched the El Capitan branch to point to mirrors.mit.edu/sage just like the other branches do.

This commit also updates broken URLs for the Mavericks and Yosemite branches.

http://mirrors.mit.edu/sage/osx/intel/sage-7.2-OSX_10.11.4-x86_64.tar.bz2

This cask also happens to symlink a binary which lives inside the moved artifact. So this is one of the cases where we might want to use the new `appdir` method that is being introduced in #13966.
6199aea
@chiselwright chiselwright added a commit to chiselwright/homebrew-cask that referenced this pull request Jun 15, 2016
@claui @chiselwright claui + chiselwright Fix triumph.rb: Remove DDP Player.app artifact (#21413)
Triumph.app contains in its bundle three helper tools. Those tools are meant to be accessed from the menu bar while using Triumph.

One of those tools, DDP Player.app, is defined as an artifact though. With upcoming PR #13966, `cask install` would rip out DDP Player.app from the bundle. As a consequence, invoking the *DDP Player* menu item would cause Triumph to crash.

This commit fixes the issue by removing the artifact altogether. All the tools (including DDP Player) should be launched from the *Tools* menu as intended, e. g.:

> Triumph » Tools » DDP Player
c99982c
@chiselwright chiselwright added a commit to chiselwright/homebrew-cask that referenced this pull request Jun 15, 2016
@mwean @chiselwright mwean + chiselwright [WIP] Change artifact behavior to moving instead of symlinking (#13966)
* Change app artifact to move instead of link

First step towards change in installation behavior mentioned in [13201]

* Fix handling of binaries linked from inside of app bundles

Also adds `appdir` method for interpolation in stanzas

* Change appdir to root Applications directory

* Update 2-app tests

* Refactor: add options, ivars to `Installer`, `Download`

In preparation for upcoming changes, this commit cleans up some code. The commit includes:

- In order to reduce unnecessary object passing, make both the `force` and `skip_cask_deps` option into instance variables of the `Installer` class

- Introduce options hashes to initializers of both the `Installer` and `Download` class

- When the `install --force` command enters the fetch phase, make it explicit in the code that fetching is never enforced in that case.

- Update tests

* Force overwrite artifacts on `--force` reinstall

This commit changes the behavior of a `Moved` artifact such that if the target already exists, `brew cask install --force` will remove the existing target before moving the staged artifact.

In that case, the warning message will say *overwriting* instead of *not moving*.

The behavior of plain `brew cask install` remains unchanged; the same goes for the warning message for that case.

* Change remaining artifacts to move instead of symlink

* Update casks to use appdir in binary paths

* Forcibly overwrite artifacts, modifying flags and using `sudo` if needed

- This commit implements [the proposed behavior for `install --force`](caskroom#13966 (comment)) when a target already exists and has either permission problems or is not owned by the user.

- The changes apply only when the `force` option is given.

- Reused the existing safeguard from the `.pkg` artifact to prevent deleting important directories by bug or mistake

- The two existing blacklists `SYSTEM_DIRS` and `UNDELETABLE_DIRS` have been consolidated into the `Hbc::MacOS` module.

- `UNDELETABLE_DIRS` now also contains all the entries from `SYSTEM_DIRS` which was a to-do anyway.

- The two blacklists are now also frozen for good measure.

- The utility method `permissions_rmtree` was moved to `Hbc::Utils`.

- The `tried_permissions` part in `Utils` now falls back correctly when there are also ownership issues at the same time.

- Introduced a separate `current_user` method for mocking.

- Added an optional feature to `FakeSystemCommand` so it can now act as a proxy to `SystemCommand`.

- Added tests for various `permissions_rmtree` cases.
5424ffd
@jawshooah jawshooah was unassigned by miccal Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment