This repository has been archived by the owner. It is now read-only.

Move launchd instructions to formula_installer. #14997

Merged
merged 2 commits into from Nov 25, 2012

Conversation

Projects
None yet
9 participants
Owner

MikeMcQuaid commented Sep 18, 2012

This isn't done yet. We probably need something for "system plists" (/Library rather than ~/Library) and possibly a way of specifying a manual command to run if you don't want to use launchd.

Any suggestions on DSL additions or functions to add for these? I'm happy to do the work I'm just undecided about the best approach here.

Member

mxcl commented Sep 18, 2012

Great.

Regarding /Library, is this actually a requirement for any tools? If the user wants the tool to run at boot, they can just remove the tildes.

Owner

MikeMcQuaid commented Sep 18, 2012

I don't think it's a requirement but there's some stuff where it only really makes sense at a system level and/or needs to run as root e.g. denyhosts

Contributor

Sharpie commented Sep 18, 2012

And some stuff, such as denyhosts, should always be run at boot. It wouldn't make sense to have the system less armored against SSH attacks until the admin account becomes active.

Member

mxcl commented Sep 18, 2012

OK, indeed. So those that should be root should advise that.

Owner

MikeMcQuaid commented Sep 18, 2012

Well exactly; those in /Library are launched at boot. Do you think we need the "how to run it manually" instructions? Any thoughts on a nice DSL?

Contributor

mistydemeo commented Sep 18, 2012

How about:

plist 'str' # for a regular user plist

plist 'str', :startup => true # for a system plist

str can be a literal string, but probably should be a method/variable like we define it right now. For example, aiccu could have:

plist startup_plist, :startup => true
Owner

MikeMcQuaid commented Sep 18, 2012

That looks pretty good, thanks Misty. Do we then want to remove startup_plist from formerly being part of the DSL?

Contributor

mistydemeo commented Sep 18, 2012

Yeah, we might as well consolidate, the same way we replaced the options method with the option DSL.

Member

mxcl commented Sep 19, 2012

Do you think we need the "how to run it manually" instructions?

Yes, I think this is mandatory because otherwise we imply there is no other way to start the app.

Related: would be nice to have such instructions for all formula, but first things first.

That looks pretty good, thanks Misty. Do we then want to remove startup_plist from formerly being part of the DSL?

Don't forget, we can't just remove stuff, taps and formula in blogs may use functions we have had for more than a few weeks. We want to maintain our reputation and not break stuff without good reason.

Member

mxcl commented Sep 19, 2012

Possibly:

plist 'str', :launch_agent => :root
plist 'str', :launch_agent => :user # optional as the default

Because :startup is a little ambiguous.

Owner

MikeMcQuaid commented Sep 19, 2012

startup_plist hasn't been part of the DSL for long so we can always add it to compatibility if necessary. I'd understand if we'd rather keep using it.

I think the launch_agent being root is confusing as it doesn't necessarily run as root.

Perhaps :launch_agent => :global, :launch_agent => :system or :launch_agent => :boot, :runs_on => :startup, :runs_on => :login?

If we want such instructions for all formula I think it probably warrants an optional DSL entry; run_command or something?

Member

mxcl commented Sep 19, 2012

plist 'str', :runs_on => :startup
plist 'str', :runs_on => :login #defaults to this

Suits. I thought the str was the "run_command". Otherwise what is it? The plist name should be Formula.name.

Owner

MikeMcQuaid commented Sep 19, 2012

It's a string with the plist contents I think; defaulting to startup_plist (hence the need for a separate run_command which I think we agree would be desirable long-term anyway).

Member

mxcl commented Sep 19, 2012

If you add run_command DSL, give it a lot of thought, or don't. Supporting these things is a burden.

run_command for instance is not a good name. It sounds like you are instructing Homebrew to run a command.

For now I'd much prefer it was part of the plist DSL, it's easy to find/replace and make a new DSL later. It's not possible to remove DSLs later.

Owner

MikeMcQuaid commented Sep 19, 2012

Ok, agreed. plist "<plist_contents>" :manual_command => 'blah', :runs_on => :startup look good?

Contributor

adamv commented Sep 19, 2012

remember that block commands can be useful:

plist something do
 blah
 blah 2
end
Contributor

jacknagel commented Sep 19, 2012

Moving the plist text to the DSL will also require adjusting the text so that it does not depend on things that are only available in the object context (i.e. any Formula methods).

Owner

MikeMcQuaid commented Sep 19, 2012

@jacknagel That sounds like a pain I'd rather avoid. @jacknagel @adamv any thoughts on syntax?

Contributor

adamv commented Sep 19, 2012

I've never plauched anything so... shrug?

Contributor

jacknagel commented Sep 19, 2012

That sounds like a pain I'd rather avoid.

Yeah. Because the DSL code is executed when a formula file is required, it doesn't even have access to the formula name (this is passed to the formula's constructor by Formula.factory). And that is really the one thing we need from object context since it is part of the plist filename and thus part of a value in the plist itself.

(It doesn't have enough information to construct cellar paths either, but this is less important now that we have opt paths that can be constructed independently of the formula prefix.)

It occurs to me that we could maybe avoid this headache by using ERB templating (like we do in brew create); this way any names in the text wouldn't be immediately bound and the template could later be evaluated in the object context with the correct bindings.

Owner

MikeMcQuaid commented Sep 19, 2012

That looks pretty neat.

Contributor

jacknagel commented Sep 19, 2012

Thanks to heredoc weirdness, it would allow something like

plist <<-EOS.undent, :startup => true
  bla bla bla
  EOS
Owner

MikeMcQuaid commented Sep 30, 2012

How about:

plist_options :startup => true, :manual => 'java jar jenkins'

def plist; <<-EOS.undent
  <xml/>
EOS

(with an alias to the old startup_plist that doesn't make much sense name-wise any more)

Owner

MikeMcQuaid commented Oct 2, 2012

Final call for format complaints (before I implement it)?

Owner

MikeMcQuaid commented Nov 6, 2012

Right, I've implemented something here. Take a look and let me know what you think. If you think it looks alright then I'll start moving the other formulae over to use it.

An outstanding question I just thought of is if we should use brew services in the caveats instead?

Owner

MikeMcQuaid commented Nov 6, 2012

Jenkins output looks like:

To have launchd start jenkins at login:
    ln -sfv /usr/local/opt/jenkins/*.plist ~/Library/LaunchAgents
Then to load jenkins now:
    launchctl load -w ~/Library/LaunchAgents/homebrew.mxcl.jenkins.plist
Or, if you don't want/need launchctl, you can just run:
    java jar jenkins
Member

mxcl commented Nov 7, 2012

Great work.

@MikeMcQuaid MikeMcQuaid merged commit 5825f62 into Homebrew:master Nov 25, 2012

Owner

MikeMcQuaid commented Nov 26, 2012

Yes, perhaps they could/should.

Are there any ways to display manual plist options after installing the package?
For example, in redis cookbook there were some caveats, which explained how to launch redis. And I did't have to remember all paths, I could just type "brew info redis" and get all paths I needed.

Owner

MikeMcQuaid commented Dec 16, 2012

Not currently; the instructions vary depending on your installation so are printed on install. I'll try and sort it for brew info at some point.

Owner

MikeMcQuaid commented Dec 16, 2012

@fxposter Let's move any future discussion of this to #16603.

Any reason why launchctl info was left here in this case?

Owner

MikeMcQuaid replied Dec 16, 2012

None, that was unintentional.

Owner

MikeMcQuaid replied Dec 16, 2012

Fixed in b5b5601, cheers.

@MikeMcQuaid Mike, why remove these instructions from caveats?

Owner

MikeMcQuaid replied Dec 30, 2012

They are printed automatically now if a plist file is detected rather than being duplicated between formulae. See formula_installer.rb at the bottom.

@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.