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

imageoptim: Only link ImageOptim.app #259

Merged
merged 1 commit into from
May 5, 2013
Merged

imageoptim: Only link ImageOptim.app #259

merged 1 commit into from
May 5, 2013

Conversation

mathiasbynens
Copy link
Contributor

Before:

$ brew cask install imageoptim
==> Downloading http://imageoptim.com/ImageOptim.tbz2
######################################################################## 100.0%
==> Success! imageoptim installed to /opt/homebrew-cask/Caskroom/imageoptim/1.4.0
==> Linking ImageOptim.app to /Users/Mathias/Applications/ImageOptim.app
==> Linking finish_installation.app to /Users/Mathias/Applications/finish_installation.app

After:

$ brew cask install imageoptim
==> Downloading http://imageoptim.com/ImageOptim.tbz2
######################################################################## 100.0%
==> Success! imageoptim installed to /opt/homebrew-cask/Caskroom/imageoptim/1.4.0
==> Linking ImageOptim.app to /Users/Mathias/Applications/ImageOptim.app

Before:

    $ brew cask install imageoptim
    ==> Downloading http://imageoptim.com/ImageOptim.tbz2
    ######################################################################## 100.0%
    ==> Success! imageoptim installed to /opt/homebrew-cask/Caskroom/imageoptim/1.4.0
    ==> Linking ImageOptim.app to /Users/Mathias/Applications/ImageOptim.app
    ==> Linking finish_installation.app to /Users/Mathias/Applications/finish_installation.app

After:

    $ brew cask install imageoptim
    ==> Downloading http://imageoptim.com/ImageOptim.tbz2
    ######################################################################## 100.0%
    ==> Success! imageoptim installed to /opt/homebrew-cask/Caskroom/imageoptim/1.4.0
    ==> Linking ImageOptim.app to /Users/Mathias/Applications/ImageOptim.app
@mathiasbynens
Copy link
Contributor Author

Perhaps it would be a good idea to only accept cask formulae that have an explicit link entry from now on.

@phinze
Copy link
Contributor

phinze commented May 3, 2013

Perhaps it would be a good idea to only accept cask formulae that have an explicit link entry from now on.

This is a good general policy question that it'd be good to get consensus on amongst the maintainers.

If you look at #257 you'll see a suggested improvement to the automatic linking code path. And if you see the #251 they decided to remove a link that was deemed extraneous.

I find myself leaning more towards your side, which would be to actually remove the automatic "link everything we find" code path and go more towards explicit linking everywhere. (If we required this to pass auditing, we'd need to provide the ability to opt out with something like a link :none, which I feel is sort of ugly at the moment, but maybe there's another way.)

@vitorgalvao, @passcod - you want to weigh in?

@mathiasbynens
Copy link
Contributor Author

Agreed with @phinze — explicitly whitelisting everything that should be linked seems like the best/safest approach.

@vitorgalvao
Copy link
Member

I very much like the simplicity of casks, so adding (making a requirement of) the tiniest change is something I’m wary of, as every addition makes the next one easier to accept, and soon it can start to become complex for new users.

However, I also believe consistency is important, and when a new user copies a cask to serve as the base to add a new one, depending on the one they copy (as it is currently) they can either add a link that is not required, or not realise that option exists, in a case that would benefit from it. For this reason alone, I think it’d be a good idea to make it a requirement (and it’s not that much extra work).

On the other hand, link :app, 'Foo.app' already looks a bit different from the rest, and is even separated by a blank line, usually, which make it clear it’s different from the other options.

In the end, I’d say I’m probably more inclined towards making it a requirement — consistency is important, and it may solve some unforeseen future issues with just linking everything by default.

@passcod
Copy link
Contributor

passcod commented May 5, 2013

Do we have any, and is there any use-case for, casks that don't have executable .apps that can be linked? No. That's kinda, like, the point, right? So no need for that link :none bullshit.

  • Explicit is great. We tried the smart route. We tried the smarter route. We tried the dumber route. It doesn't work. There's enough edge cases featuring that particular problem that at least three different people independently arrived at the conclusion it might be better if links were explicit. If none of these people are confident, we can wait at the next people to come up and say "Hey, you know, there's that problem..." and be like, okay, yeah, yeah, let's fix it, oh wait we're not actually sure we're sure we should do it yet, come on in and have a piece of cake, it's flavoured with doubt and uncertainty and we've got tea and coffee while you're waiting.
  • Linking nothing is bullshit. (Excuse my french, which is a great excuse, me being french and all.) I think I've already mentioned it. If there's nothing to be linked then WTF is there a cask for? Oh you mean this is, like, a keg-only cask, it should stay in the caskroom and not be linked outside because the beer's not finished brewing in there yet or something? Not the maintainer's decision. The guy who installs that link :none cask is going to be all like "WTF I installed this and nothing happened" and the guy who doesn't want to link stuff will be like "Oh look there's an option that says Do You Want To Link Stuff, I should probably flip that off, because I'm a psychopath/politician/CIA-agent-in-charge-of-the-Jason-Bourne-mess and I like to keep my little casks in the dark."
  • Installing software should be easy. Also, programs made by developers for an audience made of, let's be honest, a fairly significant portion of developers should be as customisable as emacs or thereabouts. And emacs might be the most horrible thing in the world, but you can play Scrabble and Halo 2 on it, and it can also do the dishes in another buffer while you're at it. On the other hand, making software installable is hard. It's hard because with the exception of specialised software (like node.js modules and rubygems and python eggs and go libraries), nobody can agree on how to package things. Hence why there's three bajillion package managers on the planet and each and every one of them has a complex packaging manifest. Seriously. If someone gets up and decides to contribute a software definition to any of these, including homebrew and cask, and then gets confused, there's a bunch of great people who are ready to walk them through the experience. It'll always be complex for new users, @vitorgalvao, remember when you started :)
  • There's a balance to achieve between having a simple format and having features that'll make casks compatible with the requirements of more projects. Myself, I'm inclined toward putting the features in, and then weeding the useless ones out upon reaching 1.0, but that's just because I like living dangerously and annoying people who use my libraries. This is apparently a more mature project than it was 6 months ago, probably because we're implementing more stuff faster. There's a contradiction somewhere in there but I'm not going to look for it, I lost my flashlight a few paragraphs up.

Now, I hope you haven't actually read all that, that would be dreadful, I'm sure I've made a fool of myself quite a few times and that a lot of it was unnecessary. You'll notice I'm still pressing the "Comment" button without deleting it all, that's how utterly hopeless I am. Have a good night.

@vitorgalvao
Copy link
Member

Well, I for one, am convinced. That was a delightful rant, @passcod, made me laugh, and I agree. It seems we have a consensus, then, that was easy.

@phinze, do you want to add anything, or may I start updating casks?

On another note, I’ll go right ahead and merge this one — it’s still a necessary change, whether link becomes obligatory or not.

vitorgalvao added a commit that referenced this pull request May 5, 2013
imageoptim: Only link ImageOptim.app
@vitorgalvao vitorgalvao merged commit b7c5c48 into Homebrew:master May 5, 2013
@mathiasbynens
Copy link
Contributor Author

\o/ Awesome thread is awesome.

@mathiasbynens mathiasbynens deleted the imageoptim branch May 5, 2013 16:37
@phinze
Copy link
Contributor

phinze commented May 5, 2013

LOL - I have nothing to add after @passcod stole the show. ;)

Let's go for it. 👍

@vincecima
Copy link
Contributor

I will update the apps I have submitted.

@vincecima
Copy link
Contributor

How are we going to enforce this, via audit?

@phinze
Copy link
Contributor

phinze commented May 5, 2013

Yeah that's what I was thinking. If we add it to audit - the build will
enforce, since the build now includes audits.

On Sun, May 5, 2013 at 1:00 PM, Vince Cima notifications@github.com wrote:

How are we going to enforce this, via audit?


Reply to this email directly or view it on GitHubhttps://github.com//pull/259#issuecomment-17455792
.

@phinze
Copy link
Contributor

phinze commented May 5, 2013

If there's nothing to be linked then WTF is there a cask for?

Oh I did come up with a reason (at least currently) for this. Some Casks wrap pkgs that we don't do anything with yet. So we need to either finish #14 or come up with a way of handling Casks like vagrant.

@passcod
Copy link
Contributor

passcod commented May 6, 2013

Mmm, yes. This feels a bit wrong, but how about a, err,

link :installer, 'SomeInstaller.pkg'

or, in some cases,

link :installer, 'SomeInstaller.app'

which would generate a "You need to run /opt/.../caskroom/.../SomeInstaller.app manually to finish this installation." ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants