feat: add the remainder of valid fields to Launchd

[dsully]

More complex code for Sockets, KeepAlive, MachServices & ResourceLimits.

Let me know if you're ok with this approach and I can add some tests.

Here's a driver program I've been using to test:

use launchd::mach_services::{MachServiceEntry, MachServiceOptions};
use launchd::sockets::SocketFamily;
use launchd::{
    BonjourTypes, KeepAliveOptions, KeepAliveType, Launchd, ProcessType, ResourceLimits, Socket,
    SocketOptions, Sockets,
};
use std::collections::HashMap;
use std::path::Path;

fn main() {
    let keep_alive = KeepAliveOptions::new()
        .with_successful_exit(true)
        .with_network_state(true);

    let socket_options = SocketOptions::new()
        .with_node_name("127.0.0.1")
        .with_family(SocketFamily::Unix)
        .with_bonjour(BonjourTypes::from(vec![
            String::from("foo"),
            String::from("bar"),
        ]));

    let socket = Socket::new("Listener", socket_options);

    let mut mach_services = HashMap::new();

    mach_services.insert("com.apple.test".to_string(), MachServiceEntry::from(true));
    mach_services.insert(
        "org.rust-lang".to_string(),
        MachServiceEntry::from(MachServiceOptions::new().reset_at_close()),
    );

    let launchd = Launchd::new("LABEL", Path::new("./foo"))
        .unwrap()
        .with_user_name("Henk")
        .with_program_arguments(vec!["Hello".to_string(), "World!".to_string()])
        .disabled()
        .with_keep_alive(KeepAliveType::from(keep_alive))
        .with_process_type(ProcessType::Background)
        .with_socket(Sockets::from(socket))
        .with_soft_resource_limits(
            ResourceLimits::new()
                .with_core(1)
                .with_data(2)
                .with_file_size(3)
                .with_stack(5)
                .with_cpu(6),
        )
        .with_mach_services(mach_services)
        .with_inetd_compatibility(true);

    launchd.to_file_xml(Path::new("./test.plist")).unwrap();
}

[koenichiwa]

Hi dsully, thank you for your contribution! Your approach looks great to me! You've added a lot of variables in these commits and I haven't touched this library in a long time, so I'll need some time to review every change you did. Anyways, thanks a lot, and let me know when you feel like you're ready.

[dsully]

It's ready to review now. I'll add tests as a follow up PR or commit.

[koenichiwa]

Hey this looks really great! Of course have some nitpicks/questions. The one I'm the most curious about is about MachServices.

Also, I'm looking forward to those tests :)

[koenichiwa]

Launchd.program is changed to Option<String>, which falls in the specs of launchd. But, if program_arguments is empty or None, then the struct is invalid. You could prevent the creation of an invalid struct by demanding at least one in the new function, but that doesn't work anymore due to derive(Default). Another solution might be something like a builder pattern with a typestate

It's not a hard requirement for this PR yet, but maybe you have a suggestion how to keep the struct valid at all times?

[koenichiwa]

I had a moment where I couldn't figure out what umask is, but it's basically unix permissions. Due to that moment I'm not sure if the type u16 is descriptive enough, but maybe just a comment would help.

[koenichiwa]

Curious if the type of inetdCompatibility is right. String in Option<HashMap<String, bool>> might be too broad. Shouldn't it be an enum of Wait (and NoWait)??

[koenichiwa]

The variables in the Launchd struct were in the same order as the manual to check if we didn't forget any variables. Now this is true up until on_demand

[dsully]

Re: program and program_arguments - I went down a number of paths on this, including having it as a ProgramTypes option, a custom serde Serializer (on the struct Launchd) and was never happy with any of those. I'll take a look at that pattern. Any ideas here are welcome.

Re: inetdCompatibility - sounds good.

Will fix the ordering.

[koenichiwa]

Re: program and program_arguments - I went down a number of paths on this, including having it as a ProgramTypes option, a custom serde Serializer (on the struct Launchd) and was never happy with any of those. I'll take a look at that pattern. Any ideas here are welcome.

I'll think about other solutions. But as I said, ensuring validity might be outside the scope of this PR. I'll come back to you on this.

inetdCompatibility - sounds good.

Great, just check if NoWait is also valid, or that the only valid key is Wait. I only found examples with Wait up until now.

Also thanks for fixing the ordering.

[koenichiwa]

One other solution might be to create a validation function? From what I remember, the only requirements where that the label was populated (I'm not sure if Mac is going to be happy with empty strings) and that either the program holds the path or the program_args

// NB: two minute draft
fn validate(&self) -> Result<(), Error> {
    if self.label.is_empty() {
        return Err(Error::InvalidState);
    }
    if self.program.is_none() {
        if let Some(args) = &self.program_arguments {
            if args.is_empty() {
                return Err(Error::InvalidState);
            }
        } else {
            return Err(Error::InvalidState);
        }
    }
    
    Ok(())
}

The problem is finding out where this function should be called... I think this is out of scope, keep it how it is and I'll create a new issue when this feature is merged.

[dsully]

Yeah - I think on Serialization is the only place.

[koenichiwa]

Looks good! I saw you added some extra variables. Also some that are deprecated, for reading purposes, which is good! I missed the ServiceIPC variable. I get that the description tells you not to use it:

ServiceIPC
Please remove this key from your launchd.plist.

But maybe we should add it for reading purposes as well.

Oh and I was curious, I've seen the trait Eq being added and removed a couple of times if I remember correctly. What was the reason?

Otherwise, great!!

[dsully]

Once I added plist::Value in the type for LaunchEvents, it doesn't implement Eq, so I had to remove it again.

[koenichiwa]

Thanks for the changes!

[koenichiwa]

Looks good! I saw you added some extra variables. Also some that are deprecated, for reading purposes, which is good! I missed the ServiceIPC variable. I get that the description tells you not to use it:

ServiceIPC
Please remove this key from your launchd.plist.

But maybe we should add it for reading purposes as well.

Oh and I was curious, I've seen the trait Eq being added and removed a couple of times if I remember correctly. What was the reason?

Otherwise, great!!

[dsully]

Let me know when you have time for another pass at this. Thanks

[dsully]

Gentle nudge. 😄

[dsully]

Ping.. on summer holiday?

[koenichiwa]

Oh my god I'm so sorry, no I was busy with my thesis, and it must've slipped my mind. I'll do this first thing today.

[koenichiwa]

Hey I did a pass over this. It looks good to me. I'll merge and put out a new version. I'm really sorry for the wait

[dsully]

Thanks - no problem, figured you were busy with life.

I'll take a look at the program / program_arguments when I have some time.

[koenichiwa]

I'll take a look at the program / program_arguments when I have some time.

Alright thanks! I'm also looking into options of deriving a builder type. I haven't found a suitable choice yet. Currently the best option is typed-builder, but it doesn't seem very mature and still comes with a lot of issues. I'll create a specific issue for this problem.

I also needed to create a "bandaid" for launchevents. It wouldn't work with every feature combination. I should've found this before merging, but alas, I thought the amount of work you've done warranted a new version for now.