Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change artifact behavior to moving instead of symlinking #13966

Merged
merged 9 commits into from May 31, 2016

Conversation

@mwean
Copy link
Contributor

@mwean 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
Copy link

@RomainLanz RomainLanz commented Nov 25, 2015

Any news on this?

@mwean
Copy link
Contributor Author

@mwean mwean commented Nov 25, 2015

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

self.class.artifact_english_name
end

def self.link_type_english_name

This comment has been minimized.

@jawshooah

jawshooah Dec 10, 2015
Contributor

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

each_artifact do |artifact|
load_specification(artifact)
next unless preflight_checks
add_altname_metadata

This comment has been minimized.

@jawshooah

jawshooah Dec 10, 2015
Contributor

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

This comment has been minimized.

@jawshooah

jawshooah Dec 15, 2015
Contributor

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).

def existing_altname_metadata
result = @command.run(
'/usr/bin/xattr',
:args => ['-p', ALT_NAME_ATTRIBUTE, target],

This comment has been minimized.

@jawshooah

jawshooah Dec 10, 2015
Contributor

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

@vitorgalvao
Copy link
Member

@vitorgalvao vitorgalvao commented Jan 3, 2016

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

@mwean
Copy link
Contributor Author

@mwean 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 mwean force-pushed the mwean:master branch 2 times, most recently to 428d57a Feb 1, 2016
@mwean
Copy link
Contributor Author

@mwean 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
Copy link
Contributor

@adidalal 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 DON'T MERGE - Change app artifact to move instead of link [WIP] Change artifact behavior to moving instead of symlinking Feb 1, 2016
@jawshooah
Copy link
Contributor

@jawshooah jawshooah commented Feb 2, 2016

Will take another look at this tonight.

@ptb
Copy link
Contributor

@ptb 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
Copy link
Contributor Author

@mwean mwean commented Feb 9, 2016

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

@ptb
Copy link
Contributor

@ptb 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
Copy link
Member

@vitorgalvao vitorgalvao commented Feb 9, 2016

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
Copy link
Contributor Author

@mwean 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
Copy link
Member

@vitorgalvao vitorgalvao commented May 17, 2016

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
Copy link
Contributor

@toonetown toonetown commented May 17, 2016

@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.

@@ -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.

This comment has been minimized.

@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)

This comment has been minimized.

@vitorgalvao

vitorgalvao Jun 1, 2016
Member

This one I have already fixed yesterday.

@@ -53,7 +53,7 @@ Easy peasy:
$ brew cask uninstall google-chrome
```

This comment has been minimized.

@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)

This comment has been minimized.

@vitorgalvao

vitorgalvao Jun 1, 2016
Member

This one I have already fixed yesterday as well.

@mwean
Copy link
Contributor Author

@mwean 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

@vitorgalvao
Copy link
Member

@vitorgalvao vitorgalvao commented Jun 1, 2016

Fixing issues caught by @muescha in #21597.

@toonetown
Copy link
Contributor

@toonetown toonetown commented Jun 1, 2016

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!!!

@vitorgalvao vitorgalvao mentioned this pull request Jun 11, 2016
1 task done
chizmw added a commit to chizmw/homebrew-cask that referenced this pull request Jun 15, 2016
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 Homebrew#13966.
chizmw added a commit to chizmw/homebrew-cask that referenced this pull request Jun 15, 2016
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 Homebrew#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
chizmw added a commit to chizmw/homebrew-cask that referenced this pull request Jun 15, 2016
…rew#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`](Homebrew#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.
vitorgalvao added a commit that referenced this pull request Oct 7, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet