command line separation with -- broken in several context

[borkdude]

Given a script cmd.clj:

$ cat /tmp/cmd.clj
(prn *command-line-args*)

The following should not have the hyphen probably?

$ bb -f /tmp/cmd.clj -- asdf sdf
("asdf" "sdf")

$ bb /tmp/cmd.clj -- asdf sdf
("--" "asdf" "sdf")

$ bb -- asdf sdf <<< '*command-line-args*'
nil

$ bb repl -- asdf sdf <<< '*command-line-args*'
----- Error --------------------------------------------------------------------
Type:     java.lang.Exception
Message:  File does not exist: asdf

[bobisageek]

I agree (I think) that in all of these cases, the -- should not be an arg, and *command-line-args* should align with the first example.
The repl example is sort of interesting, but I guess at the end of the day, the idea of passing args into a REPL isn't really any different.

[felixdo]

Hmm, we have -- in use in git, also in kubectl. For those to work I guess they need -- as an arg no?

[bobisageek]

Regarding scenario 2 (file name without -f), this appears to be the special handling for files in main/parse-opts. If the arg is a file,

babashka/src/babashka/main.clj

Line 751 in 5cd2b9d

(parse-file-opt options opts-map)

gets called, which puts everything after the file name (including --) into :command-line-args.
The first idea that comes to mind is doing a recur thing sort of like how subcommands are handled, except maybe assoc'ing :file onto opts-map, and recurring with (next options) and the updated opts-map. I'm not too sure yet if that has unwanted consequences, because parse-file-opt is called from two places, so tweaking that could be problematic.

There's another potential scenario, which is a file that doesn't exist (bb i-dont-exist.clj -- asdf), and can get set to :file if we get to the fallthrough at

babashka/src/babashka/main.clj

Lines 680 to 684 in 5cd2b9d

	(assoc opts-map
	(if (str/ends-with? opt ".jar")
	:jar
	:file) opt
	:command-line-args (next options)))))))

I think this is actually what's happening in scenario 4, because if I'm working this out correctly,

babashka/src/babashka/main.clj

Lines 614 to 618 in 5cd2b9d

	("--repl")
	(let [options (next options)]
	(recur (next options)
	(assoc opts-map
	:repl true)))

means that when --repl (or repl which gets re-written to --repl), this code consumes both --repl and the option after it, which in case of scenario 4 means the -- is getting swallowed. Adding a throw-away option eliminates the file not found error:

$ bb repl -- asdf sdf
----- Error --------------------------------------------------------------------
Type:     java.lang.Exception
Message:  File does not exist: asdf

$ bb repl junk -- asdf sdf
Babashka v1.3.190 REPL
...

The fact that *command-line-args* is nil is (I think) sort of orthogonal, and I think it's tied to core/command-line-args only being bound in the expressions path in main

babashka/src/babashka/main.clj

Line 1048 in 5cd2b9d

core/command-line-args command-line-args]

[borkdude]

@bobisageek If you want to work on a PR to address (some of) these, you're welcome to do so.

[bobisageek]

I'm trying to think through some of the possibilities where a change here could change existing behavior in an undesired way.

There are a couple existing tests

babashka/test/babashka/main_test.clj

Lines 77 to 78 in 5cd2b9d

	(is (= ["--version"] (bb nil (fs/file "test-resources" "script_with_overlapping_opts.clj") "--version")))
	(is (= ["version"] (bb nil (fs/file "test-resources" "script_with_overlapping_opts.clj") "version")))))

that I think sort of imply "if the first argument is a file (with no -f), then everything afterwards is an arg to the file".
Put a different way, 'fixing' scenario 2 might break those existing tests (and obviously, scripts that rely on that).
If we sort of flip scenario 2 around to match those tests:

; this is the existing test - pass a file without `-f` and we don't process any more args...
(main/main "test-resources/script_with_overlapping_opts.clj" "--version")
("--version") ; this is the prn of *command-line-args*
=> 0 ;exit code

; but if we use `-f`, then subsequent args are still processed as potential args to `bb`
(main/main "-f" "test-resources/script_with_overlapping_opts.clj" "--version")
babashka v1.3.191-SNAPSHOT ; just print the version text, script effectively does nothing
=> 0

So, as a solution that I think would preserve that existing behavior, we could special-case that if we're 'inferring' a file (as opposed to being told with -f), then remove -- if it's the first argument after the file name. I'm sort of not crazy about that approach (feels a little dirty to me).
More generally, I wonder if we want to think about the with-and-without -f scenarios working the same way. This might fix scenario 2, but I think would break the existing test.
I'd like to get some feedback before pushing too far down one road or the other, because it feels like a trade-off situation.

[borkdude]

OK, we can ignore changing -f right now I guess. The real issues were the latter two in the original post. Let's just focus on those then.

[borkdude]

Somehow the docker step on master started acting up after merging this. Not sure if it's related.

[bobisageek]

The Github action 'docker' looks like it's disabled. Is there a different docker step I should be looking at?

[borkdude]

CircleCI

[borkdude]

Oh I see:

:err "curl: (23) Failed writing received data to disk/application\n

This has nothing to do with your PR.