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

launchd: only support setting command to a list #1088

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Enzime
Copy link
Collaborator

@Enzime Enzime commented Sep 22, 2024

@emilazy should I just deprecate setting command to a string with a warning?

@emilazy
Copy link
Collaborator

emilazy commented Sep 22, 2024

Deprecating that seems reasonable, but I’m a bit worried that it means the obvious translation of prog $a to [ "prog" "$a" ] behaves differently because of the escaping. I guess those cases can use script instead? Maybe we should use another name for this variant and just deprecate command entirely?

@Enzime
Copy link
Collaborator Author

Enzime commented Sep 22, 2024

I was thinking of changing the types to any options that set extra arguments to lists as well so that we can run them through lib.escapeShellArgs as well

@Enzime Enzime force-pushed the command-list branch 2 times, most recently from 2420824 to e2af976 Compare September 29, 2024 04:41
@Enzime Enzime changed the title launchd: support setting command to a list launchd: only support setting command to a list Sep 29, 2024
@Enzime Enzime force-pushed the command-list branch 4 times, most recently from 7398e9b to ba765fa Compare September 29, 2024 10:37
Copy link
Collaborator

@emilazy emilazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. There’s a test failure; do you know if it’s related?

@@ -80,11 +72,11 @@ let
};

config = {
command = mkIf (config.script != "") (pkgs.writeScript "${name}-start" ''
command = mkIf (config.script != "") [ (pkgs.writeScript "${name}-start" ''
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a wait4path in here? (I’m fine if you’d rather leave that to future work.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary as serviceConfig.ProgramArguments already does wait4path /nix/store

@@ -91,7 +92,7 @@ in
# How often AutoSSH checks the network, in seconds
environment.AUTOSSH_POLL="30";

command = "${pkgs.autossh}/bin/autossh -M ${toString mport} ${s.extraArguments}";
command = [ (lib.getExe pkgs.autossh) "-M" "${toString mport}" ] ++ s.extraArguments;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is the only place where we pass arbitrary user arguments in this field. After this change users will have to adjust their configurations and wouldn’t be able to reference environment variables here any more. I’m okay with it if you are, but I’m guessing we might have services for which this is undesirable in future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In those situations, I think we should direct them to set serviceConfig.ProgramArguments directly (possibly with lib.mkAfter)

I did a quick GitHub Code search and didn't see any configs that used environment variables for services.autossh.extraArguments:

https://github.com/search?utf8=✓&q=path%3A*.nix+services+autossh+extraArguments&type=Code

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

Successfully merging this pull request may close these issues.

2 participants