Refactor user-defined scripts

[jpdillingham]

Closes #1278

Documentation to follow once folks have had a chance to confirm functionality

Testing

Linux

• 'command' mode
• 'args' mode
• 'args list' mode
• executable + args

Windows

• 'command' mode
• 'args' mode
• 'args list' mode
• executable + args

[boyska]

why is this called command? I understand this is what is usually called args (except it's a single string instead of a list of strings, which is a more typical way of doing it).

[boyska]

and why isn't data/my_script.sh passed in the executable field? not that it would really make a difference...

[jpdillingham]

my goal was to get to semantics that "just work" as if someone was at their terminal typing in a prompt. i think i still missed the mark because this needs -c or /c as a first argument to work that way.

i'll split this out into shell and a combination of executable and args. shell will use the existing logic to determine the system shell and automatically prepend -c or /c, while the combination of executable and args will give power users maximal control

[boyska]

I'm not sure this is the best idea, but if you really think you should, then you should also go into the string quoting headache.

Let's assume you want to do something very simple, like this

echo "The new value is $(echo -n "$SLSKD_SCRIPT_DATA" | jq '.foo[]|.bar')" >> /tmp/slskd-log.txt

This needs two different quoting: one is done by the user, when writing the YAML configuration. It sucks, but it's not your problem as a developer.

The second kind of quoting happens when you want to invoke such a line. I see you can't have ArgumentList, so you're left with a single string. So you would need to do some form of quoting so that ProcessStartInfo splits your command the correct way. (ie: ["/bin/sh", "-c", "whatever I wrote earlier"]). I don't know how that works exactly, and I wouldn't be surprised if that was platform-specific.

[boyska]

I know nothing about csharp (so feel free to ignore this comment), but isn't ArgumentList just more practical than Arguments?

[jpdillingham]

it would be but unfortunately there's only Arguments which is a string

[boyska]

What about that? https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.processstartinfo.-ctor?view=net-9.0#system-diagnostics-processstartinfo-ctor(system-string-system-collections-generic-ienumerable((system-string))) Again, sorry if I'm talking gibberish: I really know nothing about the .net environment

[boyska]

that's sooo much nicer interface!

is there any case in which using a single string (args) should be recommended for user? shouldn't args_list just be enough for everyone, allowing for the code to be simplified a lot?

[jpdillingham]

My big concern with this is that if args were to take on the behavior of args_list (which I've just renamed to arglist to better match other applications) people would fumble trying to specify a string where an array is expected. YAML formatting errors get really ugly and cryptic error messages and I see users struggle with it a lot, so I'm trying my best to make it as intuitive as possible

[boyska]

people would fumble trying to specify a string where an array is expected

I see your point. My experience, as a user, is that I learned why "strings as command arguments don't work" the hard way: I was trying to write commands in similar settings, and that was just repeatedly buggy. And whenever I was asking developers what I should do to handle the n-th level of quoting, the only meaningful tip I got was "just create a wrapper script and invoke that". So, since then, my thought is always: why isn't this just stated upfront since the beginning? Why are examples pointing me in a direction which is fragile and sometimes just not working? Why do people mess with placeholder in argument strings (which are difficult to handle) compared to passing data to stdin (which is straightforward)?

Anyway, back to the case at hand, I see how your concern is an important one, too. I still stand that it would be better to just have args_list. I think you are right in pointing out that users could get confused. Good mitigations are:

• provide clear examples
• provide a clear error message, like "args_list should be a list of arguments, not a string. See link.to/doc/ for examples"

I think you are still headed to have this triple mode merged. I think it's making the code more complex for no real value, but you're the maintainer, so it's your call :)

[boyska]

Wouldn't this break if the user supplied a command which includes double quotes? ie:

touch "/tmp/notify me"

[jpdillingham]

it didn't when i was testing on Linux last night. i'm going to test on Windows this morning

[jpdillingham]

Broke on Windows; I pushed a fix. Thanks for pointing it out.

[jpdillingham]

I'm going to push this out to the canary so I can test in a wider variety of scenarios. The final structure is:

• 'Command mode': specify only a single string that will be passed as an argument to your system shell prefixed by -c or /c depending on your OS. This should work more or less like a command line, and users are on the hook to get quotes right.
• 'Args mode': specify an executable (or not) and a single string that will be passed as an argument. I'm anticipating that this would be used with a third party executable (not a shell) and users have full control over the args. This is included as a 'backstop' to help users that may rely on their intuition (rather than the docs) to help them configure scripts.
• 'Args list mode': specify an executable (or not) and an array of arguments. Users are not on the hook for getting quotes right. This is the recommended way to configure a script with non-trivial args.

I'm going to hold off on updating the docs until I've verified that things are still working as expected.

[jpdillingham]

Thanks @SevereOverfl0w and @boyska for the feedback! Please let me know what you think of the finished product.