allow "attach" and "down" subcommands to work, show it in ponysay example

[VanCoding]

As a follow up to my last comment here: #81 (comment)

Currently it's not possible to use the "attach" and "down" (and probably all other) subcommands, because they don't allow the "--config" argument, which currently is passed all the time.

This PR is a possible fix to this, with the following drawback: explicitly setting the "up" command will not work, and it also won't work to pass any additional parameters.

I originally wanted to propose the following solution, which would be a bit better, but still not perfect:
VanCoding@5b66d08

That solution would require the subcommand to be the first parameter. But that's probably something the users could live with.

[VanCoding]

Dang, my first attempt also didn't work, because the test always adds arguments and therefore the wrapper thought it's not the "up" command.

I've committed the possible solution. But if you prefer to go with the previous strategy, then the "-t=false" argument probably needs to be added inside default.nix, where also the config is swapped out. That's another parameter that gets overridden for the test, so maybe the separate options for the test process weren't that bad after all :/

[srid]

In the "$1" == "up" case, shouldn't you drop the up from $@? Otherwise, in line 60 it will be passed twice.

[VanCoding]

No, we're passing the arguments on as we receive them, and we never add an "up" ourselves afaik

[srid]

You are right:

❮ nix run --override-input process-compose-flake .. .#default -- up --log-file=output.log
[...]
+ process-compose --no-server --config /nix/store/bl46rll1p24g5fbdypbxsk6rfnklfp2z-process-compose-default.json --disable-dotenv --theme Cobalt up --log-file=output.log

But then, I just noticed something new here. Note that --disable-dotenv --theme Cobalt (defined in cli.up) appears before the up argument. It looks like their CLI parser accepts this out of order arguments anyway. But for clarity, I'd prefer that --disable-dotenv --theme Cobalt appear after the up argument, because those options are meant for up subcommand anyway.

Suppressing incorrect observation

Wait, I was wrong. Actually those are global options! We should fix this in options.

---

--sort however is an up specific option, so that's a better illustration is another example:

--- a/example/flake.nix
+++ b/example/flake.nix
@@ -30,8 +30,7 @@
               };
               # Options for `process-compose up`
               up = {
-                disable-dotenv = true;
-                theme = "Cobalt";
+                sort = "health";
               };
             };
             settings = {

Without arguments, we see:

❯ nix run --override-input process-compose-flake .. .#default
[..]
+ process-compose --no-server --config /nix/store/bl46rll1p24g5fbdypbxsk6rfnklfp2z-process-compose-default.json --sort health

(And I'd still like to make up implicit here)

With up argument:

❯ nix run --override-input process-compose-flake .. .#default up
+ process-compose --no-server --config /nix/store/bl46rll1p24g5fbdypbxsk6rfnklfp2z-process-compose-default.json --sort health up

Here, I think --sort health should appear after up to make it clear (to the user who happens to diagnose our script output) that these flags are for up command only.

[VanCoding]

I agree, it's better to put user arguments first. I've changed that.

[srid]

Found some bugs:

--help breaks:

❯ nix run --override-input process-compose-flake .. .#ponysay --  --help
warning: Git tree '/Users/srid/code/process-compose-flake' is dirty
warning: Git tree '/Users/srid/code/process-compose-flake' is dirty
warning: not writing modified lock file of flake 'git+file:///Users/srid/code/process-compose-flake?dir=example':
• Updated input 'process-compose-flake':
    'github:Platonic-Systems/process-compose-flake/d0824dab6ff49a06ed10e48177aabaef52469a0a' (2024-09-30)
  → 'git+file:///Users/srid/code/process-compose-flake' (2024-10-14)
+ process-compose --help --config /nix/store/ysw18q5xn937rlf8pf7zi2xjrwmcpmpd-process-compose-process-compose.json --detached
Error: unknown command "/nix/store/ysw18q5xn937rlf8pf7zi2xjrwmcpmpd-process-compose-process-compose.json" for "process-compose"
Run 'process-compose --help' for usage.

[srid]

I don't see what purpose [[ "$1" == -* ]] serves here.

[VanCoding]

I tried to explain it in the comments, but it looks like it failed 😅

If the first argument starts with a dash, we know it's no subcommand. And if the first argument is no subcommand, we assume there is no subcommand argument at all. And if there is none, process-compose defaults to the up command. And therefore it's allowed to pass arguments dedicated to the up command.

[srid]

I understood the what, but I'm asking why.

If the first argument starts with a dash, we assume there isn't a subcommand, so it's also the "up" command

This is of course wrong, since --help is not equivalent to up.

[VanCoding]

Yes, that seems to be one of the edge cases I mentioned. Properly dealing with it could be tricky, though. Any ideas?

[srid]

Why not remove [[ "$1" == -* ]] entirely?

[VanCoding]

Found some bugs:

--help breaks:

❯ nix run --override-input process-compose-flake .. .#ponysay --  --help
warning: Git tree '/Users/srid/code/process-compose-flake' is dirty
warning: Git tree '/Users/srid/code/process-compose-flake' is dirty
warning: not writing modified lock file of flake 'git+file:///Users/srid/code/process-compose-flake?dir=example':
• Updated input 'process-compose-flake':
    'github:Platonic-Systems/process-compose-flake/d0824dab6ff49a06ed10e48177aabaef52469a0a' (2024-09-30)
  → 'git+file:///Users/srid/code/process-compose-flake' (2024-10-14)
+ process-compose --help --config /nix/store/ysw18q5xn937rlf8pf7zi2xjrwmcpmpd-process-compose-process-compose.json --detached
Error: unknown command "/nix/store/ysw18q5xn937rlf8pf7zi2xjrwmcpmpd-process-compose-process-compose.json" for "process-compose"
Run 'process-compose --help' for usage.

Interesting.. I'll have to look into that. But I think we won't be able to make it work for 100% of the cases, we would have to build a proper wrapper for that, that can parse the arguments the same way process compose does.

[srid]

But I think we won't be able to make it work for 100% of the cases, we would have to build a proper wrapper for that, that can parse the arguments the same way process compose does.

Or you can just remove the [[ "$1" == -* ]] part, no? What specific use case does it serve, anyway?

[VanCoding]

But I think we won't be able to make it work for 100% of the cases, we would have to build a proper wrapper for that, that can parse the arguments the same way process compose does.

Or you can just remove the [[ "$1" == -* ]] part, no? What specific use case does it serve, anyway?

It's there to be compliant with the original CLI. If you don't pass "up" there, it still will start the processes. It will still try to do that if we remove that part, but it won't get any arguments (like --config) and therefore fail.

[srid]

Can you be a little more concrete?

Specifically, for this diff,

--- a/nix/process-compose/default.nix
+++ b/nix/process-compose/default.nix
@@ -52,7 +52,7 @@ in
             # Otherwise, we assume it's a subcommand other than "up"
             params=(${cliOutputs.global})
             set +u
-            if [ -z "$1" ] || [[ "$1" == "up" ]] || [[ "$1" == -* ]] ; then
+            if [ -z "$1" ] || [[ "$1" == "up" ]] ; then
               params+=(--config ${configFile} ${cliOutputs.up})
             fi

... which particular invocation of the example is problematic (such as to warrant the justification of [[ "$1" == -* ]])?
This is what I was asking here: #82 (comment)

For example, I ran it without any arguments, and it works just fine:

❯ nix run --override-input process-compose-flake .. .#ponysay
warning: Git tree '/Users/srid/code/process-compose-flake' is dirty
warning: Git tree '/Users/srid/code/process-compose-flake' is dirty
warning: not writing modified lock file of flake 'git+file:///Users/srid/code/process-compose-flake?dir=example':
• Updated input 'process-compose-flake':
    'github:Platonic-Systems/process-compose-flake/d0824dab6ff49a06ed10e48177aabaef52469a0a' (2024-09-30)
  → 'git+file:///Users/srid/code/process-compose-flake' (2024-10-14)
trace: evaluation warning: getExe: Package ponysay-unstable-2021-03-27 does not have the meta.mainProgram attribute. We'll assume that the main program has the same name for now, but this behavior is deprecated, because it leads to surprising errors when the assumption does not hold. If the package has a main program, please set `meta.mainProgram` in its definition to make this warning go away. Otherwise, if the package does not have a main program, or if you don't control its definition, use getExe' to specify the name to the program, such as lib.getExe' foo "bar".
+ process-compose --config /nix/store/ysw18q5xn937rlf8pf7zi2xjrwmcpmpd-process-compose-process-compose.json --detached
Starting Process Compose in detached mode. Use 'process-compose attach' to connect to it or 'process-compose down' to stop it
+ set +x

[VanCoding]

Ah, yeah, I see where the confusion is coming from now. It's all fine if you don't pass any parameters.Because of [ -z "$1" ]. But as soon as you pass one, like for example --detached (not through options!) it won't receive --config anymore, and fail.

So try running nix run --override-input process-compose-flake .. .#ponysay --detached and you should see what I mean.

[srid]

Let's just have the user explicitly pass up --detached as arguments.

nix run --override-input process-compose-flake .. .#ponysay -- up --detached

[VanCoding]

Yes, I also think it's a good idea to require explicitly providing the subcommand. But shouldn't we communicate this to the user somehow, so that they know what's going on?

And for consistency, I think it's best to then also require "up" when no arguments get passed. So, nix run --override-input process-compose-flake .. .#ponysay also shouldn't work and also should bring a message about providing a subcommand.

[VanCoding]

I've implemented my suggested solution and I'm personally pretty happy with it. Apart from requiring a subcommand as the first argument, we're fully compatible with all other argument constellations this way.

[srid]

Upstream already uses up as default when none is provided:

❯ , process-compose --config /nix/store/ysw18q5xn937rlf8pf7zi2xjrwmcpmpd-process-compose-process-compose.json
24-10-20 08:23:50.432 FTL start http server on :8080 failed error="listen tcp :8080: bind: address already in use"

We do the same of course. nix run .#foo (without arguments) should just work in that it should spin up the processes. But the last commit broke it:

❯ just ex
cd ./example && nix run --override-input process-compose-flake ..
warning: not writing modified lock file of flake 'git+file:///Users/srid/code/process-compose-flake?dir=example':
• Updated input 'process-compose-flake':
    'github:Platonic-Systems/process-compose-flake/d0824dab6ff49a06ed10e48177aabaef52469a0a' (2024-09-30)
  → 'git+file:///Users/srid/code/process-compose-flake?ref=refs/heads/VanCoding/main&rev=58d976ed053413421ac478debe0d4bdbcb38f1b8' (2024-10-20)
process-compose-flake requires a subcommand like 'up' as the first argument. Configured subcommand cli options are ignored otherwise.
To get a list about available subcommands, use the 'help' subcommand
error: Recipe `ex` failed on line 19 with exit code 1

[VanCoding]

Upstream already uses up as default when none is provided

I'm aware of that, and the previous version took pretty good care of that. Then we decided to break upstream and require specifying a subcommand.

If the goal is to not break things, we can just revert.

[srid]

Then we decided to [..] require specifying a subcommand.

I never agree to that, no.

If the goal is to not break things, we can just revert.

I'm okay with breaking backwards compat if there's significant value in the change; in this case, there's no value whatsoever (if only, it adds to inconvenience of the user).

[VanCoding]

I never agree to that, no.

You said

Let's just have the user explicitly pass up --detached as arguments.

That's a breaking change. Upstream did allow passing arguments without providing the subcommand.

[srid]

Oh, I see the misunderstanding. What I meant was that if -- and only if -- the user is going to pass options specific to the up subcommand, they should explicitly pass the subcommand (up) as well.

i.e., don't support:

nix run .#myservice -- --detached

but do support:

nix run .#myservice -- up --detached

(especially as explicit is better than implicit)

all the while, the following still works as before:

nix run .#myservice

as does:

nix run .#myservice -- down # or attach, etc.

which this PR adds support for.

Upstream did allow passing arguments without providing the subcommand

If you mean that process-compose --detached automatically works, interpreting it as process-compose up --detached ... I personally consider it a bad form of automagic, and we shouldn't care to support it if it will complicate our code (but see below).

---

By the way, it just occurred to me that that --config replaced PC_CONFIG_FILES in #81 and if he hadn't done that, all of this (the argument processing I mean) would have been much simpler.

[srid]

By the way, it just occurred to me that that --config replaced PC_CONFIG_FILES in #81 and if he hadn't done that, all of this (the argument processing I mean) would have been much simpler.

So it looks like we should just go back to PC_CONFIG_FILES ... what do you think? Then we can pass $@ as is without further processing (except to see if up is in it, and then pass the up args from Nix, etc.). And this would enable the <prog> --detached case to work.

[VanCoding]

I think --config vs PC_CONFIG_FILES is a different topic, that not really has something to do with this. And I'll try my best to explain why:

The actual problem we're dealing with, is that process-compose complains when arguments are passed to a subcommand the arguments are not meant for. So if we pass --config or --no-deps to the down subcommand, it won't work. While it's true that process-compose does not complain, if the arguments are passed through their env-var variant instead, we still have the problem, that not all arguments have an env-var variant, and those still need to be passed as normal args. And for these, we still need to detect the subcommand.

Now back to the previous topic:

If you mean that process-compose --detached automatically works, interpreting it as process-compose up --detached ... I personally consider it a bad form of automagic, and we shouldn't care to support it if it will complicate our code (but see below).

I understand that you don't want to overcomplicate code. That's reasonable.
What I don't understand is why breaking process-compose is a problem, but breaking process-compose --no-deps is not. Both did work before, and both work without our wrapper as well. I don't add any magic functionality, I just try to be as close to the original CLI as possible.

And, frankly, as a user, I'd be really confused why process-compose works, and process-compose --no-deps does not, especially if I already know the original CLI and use it like this daily.

So, the question seems to be where to draw the line between not breaking and keeping the code simple. You seem to have found that answer already with just not supporting any additional args when no subcommand is specified. For me it's the code how it currently is, since it adds a bit more compatibility and isn't really that much more complex (IMO of course).

I think the issue is clear now, including the possible solutions and their pros & cons. And it's probably fine to have different opinions on this. Let's just make a decision and tick it off :)

[srid]

I appreciate the explanation. I agree with most of it.

While it's true that process-compose does not complain, if the arguments are passed through their env-var variant instead, [..]

Yes, I believe using PC_CONFIG_FILES will obviate the problem entirely.

[..] we still have the problem, that not all arguments have an env-var variant, and those still need to be passed as normal args. And for these, we still need to detect the subcommand.

You seem to have misinterpreted my suggestion. I suggested using env var only for the config file (ie., PC_CONFIG_FILES over --config), but not for all other options that are normally passed in the CLI.

However, our problem here is that with this design it becomes impossible to apply cliOutputs.up in all valid scenarios, i.e, that auto-magic pc --detached case (without the explicit subcommand, up). This wasn't an issue prior to #81, so this conundrum is making me evaluate if we needed all of those CLI options to be defined in Nix. What is the value of doing detached = true; in your flake, as opposed to just using --detached, or wrapping the process-compose package in custom flake app (see #27)?

To conclude, we should probably get rid of the up module options (keeping only global ones), and go back to using PC_CONFIG_FILES and PC_DISABLE_TUI.

The original logic was:

The new one would be similar to this:

export PC_CONFIG_FILES=${configFile}
# This includes PC_DISABLE_TUI
${lib.concatStringsMapSep "\n" (k: v: "export $k=$v") config.extraEnv)
${preHook}
set -x; process-compose ${cliOutputs.global} "$@"; set +x

Which is much simpler overall.

[VanCoding]

@srid oh, ok if all up-arg options are removed again it's a different story, sure. It would be sad to not have these options, though. There are some useful ones like --env, --detached, --theme, or --ref-rate after all. But I see why you're in doubt about keeping these around for the sake of simplicity.

In the end, fortunately, the wrapper can be wrapped again to re-add this logic, which I'd probably do then for my projects.

[srid]

Yup, let's do that.

[srid]

Yup, let's do that.

#84