Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Move launchd instructions to formula_installer. #14997

Merged
merged 2 commits into from

9 participants

@mikemcquaid
Owner

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.

@mxcl
Collaborator

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.

@mikemcquaid
Owner

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

@Sharpie
Collaborator

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.

@mxcl
Collaborator

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

@mikemcquaid
Owner

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?

@mistydemeo
Owner

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
@mikemcquaid
Owner

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

@mistydemeo
Owner

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

@mxcl
Collaborator

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.

@mxcl
Collaborator

Possibly:

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

Because :startup is a little ambiguous.

@mikemcquaid
Owner

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?

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

@mikemcquaid
Owner

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

@mxcl
Collaborator

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.

@mikemcquaid
Owner

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

@adamv
Owner

remember that block commands can be useful:

plist something do
 blah
 blah 2
end
@jacknagel
Collaborator

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

@mikemcquaid
Owner

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

@adamv
Owner

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

@jacknagel
Collaborator

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.

@mikemcquaid
Owner

That looks pretty neat.

@jacknagel
Collaborator

Thanks to heredoc weirdness, it would allow something like

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

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)

@mikemcquaid
Owner

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

@mikemcquaid
Owner

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?

@mikemcquaid
Owner

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
@mxcl
Collaborator

Great work.

@mikemcquaid mikemcquaid merged commit 5825f62 into Homebrew:master
@mikemcquaid
Owner

Yes, perhaps they could/should.

@fxposter

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.

@mikemcquaid
Owner

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.

@mikemcquaid
Owner

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

@asymmetric

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

None, that was unintentional.

Fixed in b5b5601, cheers.

@karmi

@mikemcquaid Mike, why remove these instructions from caveats?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.