Fix splitArgs function.

[pranaysashank]

• Closes --test-options are split wrong #8090.

TODO

• Check compatibility with Bad escaping of --test-options #6248.

---

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

[robx]

Just in case you didn't see: There's a second CI failure in the package config roundtrip tests, which I can't quite make sense of, but they're all of the form

program-options
    c2hs-options: "
"

(As I understand it that's generated as a valid config, but no longer parseable. Not sure that should be a valid config...)

[pranaysashank]

As I understand it that's generated as a valid config, but no longer parseable. Not sure that should be a valid config...

At least cabal reject that config with the same error that we see in the CI, so it's not a valid config. We are getting this error while shrinking which means something else failed before.

I disabled shrinking and got an error which is related to this pr i.e. single quotes have to be paired now. This is the failure:

-- new behaviour
> splitArgs  "G%D PP X#' # njJ"
["G%D","PP","X# # njJ"]
-- old behaviour
> splitArgs "G%D PP X#' # njJ"
["G%D","PP","X#'","#","njJ"]

We need to decide what to do here

[Mikolaj]

I'm getting

~$ echo don't
> ^C
~$ echo don't'
dont

so indeed, on commandline single quotes need to be matched and are special, so IMHO we are fine to treat them as such in commandline arguments, whether really coming from commandline or written in config files, etc.

[jneira]

For completeness the above test in win cmd gives diff results:

C:\Users\atrey>echo don't
don't

C:\Users\atrey>echo don't'
don't'

the behaviour in powershell is the same as sh:

PS D:\> echo don't
>> ^C
PS D:\d> echo don't'
dont

we could use sh in windows wth msys2 and the behaviour will be the same of course

[Mikolaj]

For completeness the above test in win cmd gives diff results:

Oh dear. But with double quotes it works the same as in POSIX? If so, this means cabal should only use double quotes when calling GHC and other tools, to be usable with Windows cmd. But I think, for simplicity, it should still assume POSIX for commandline that for any reason contains single quotes, e.g., because the user wrote them, as in this case.

[Mikolaj]

Nobody objected, so I'd say, let's run with the current design. On Windows, people would be restricted to double quotes, but oh well.

Let me rebase, because we've cleaned up CI, so the state of this PR should be more apparent now.

[Mikolaj]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[Mikolaj]

Nah, it seems GHC just cancels all tests after the single failure we discussed how to fix. Let's do it. :)

[Mikolaj]

But just in case let me rebase again, after GHA fixed itself and we tweaked something in CI again.

[Mikolaj]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[ulysses4ever]

I'm curious: what is the status of this? Is it ready for prime time?

[Mikolaj]

I'm curious: what is the status of this? Is it ready for prime time?

It's one tiny fix away from prime time, I hope.

[ulysses4ever]

@Mikolaj which one? You said above: "Nobody objected, so I'd say, let's run with the current design." and then rebased it a couple time, so I thought this patch already implements what has been agreed upon? There was a CI error but the author mentioned that cabal also fails in that case.

[Mikolaj]

Let me try to recall. I think you are right the current behaviour is fine, which means the fix should be to the testsuite so that it accepts the current behaviour (with some explanation somewhere, in the worst case just a comment in a test).

[ulysses4ever]

Does anyone understand the doctest failure:

 doctest --fast Cabal-syntax/src Cabal/src
Cabal/src/Distribution/Simple/Setup.hs:2265: failure in expression `splitArgs "--foo=\"C:/Program Files/Bar/\" --baz"'
expected: ["--foo=C:/Program Files/Bar/", "--baz"]
 but got: ["--foo=C:/Program Files/Bar/","--baz"]
                                          ^

Superficially, it seems like the results are the same but there's a difference in the way lists are outputted... Am I missing something?

[pranaysashank]

@ulysses4ever AFAIK doctest does a literal string equality check, it looks like there should be no space in between the elements

[ulysses4ever]

@mergify rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[ulysses4ever]

I fixed the failing test and would appreciate reviews to hopefully get it done finally.

[mergify]

rebase

✅ Branch has been successfully rebased

[Mikolaj]

@robx: are your comments taken into account?

[Mikolaj]

@pranaysashank: do you like the tweaks and the final form of this PR?

[pranaysashank]

@Mikolaj yes it looks fine and thanks @ulysses4ever for fixing the tests.

I am no longer confident that this is a bug, or that this PR is the correct way to fix it. With this PR I don't think we can any longer pass ' as part of an argument which could be quite common in function names. So this no longer works (I haven't tested it myself)

cabal test sometest --test-options "-p findRoot'"

And ways of escaping a single quote seems non trivial, so I think this PR only adds more confusion to the existing situation

[Mikolaj]

So this no longer works

Ouch. I wonder what is the prevailing mode of operation of old and new Unix commandline utilities in such cases. Which quotes are used for grouping and if/how they escape them.

[pranaysashank]

Given that

$ echo don't
> ^C
$ echo "don't"
don't

it is probably understood that to have literal ' it should be double quoted, which means they can do one of

$ echo "\"don't\""
> "don't"
$ cabal test sometest --test-options "-p 'find root' \"anotherOption'\""
or maybe
$ cabal test sometest --test-options "-p 'find root' \"\\\"anotherOption'\\\"\""

So maybe this pr is fine idk 🤷 . Once we have a way to pass arguments to test executables similar to how we pass them to run executables these issues will likely become even less relevant.

Edit:
And I don't think we made the situation worse than it was before, it'd be nice if anyone else could confirm that

[Mikolaj]

People trip over this in the wild, due their exposure to unix tools conventions. I think we should just follow these conventions, not try to evaluate them. The only problem is, we should follow them reasonably close.

$ cabal test sometest --test-options "-p 'find root' "anotherOption'""
or maybe
$ cabal test sometest --test-options "-p 'find root' "\"anotherOption'\"""

I think it would be ideal if we had tests for these cases showing that cabal passes along the same strings echo does. In fact, we could have QuickCheck tests that do this for random strings, but it would of course fail all the time for exotic cases that nobody cares about. :)

Once we have a way to pass arguments to test executables similar to how we pass them to run executables these issues will likely become even less relevant.

Could you elaborate?

[ulysses4ever]

Could you elaborate?

I have a guess: #7454 had a reasonable plan, I think. I was going to pick it up at some point (unless someone beats me to it).

[pranaysashank]

Once we have a way to pass arguments to test executables similar to how we pass them to run executables these issues will likely become even less relevant.

Could you elaborate?

Yep, basically what @ulysses4ever said. Right now when we do cabal run we pass arguments as

$ cabal run sometest -- --arg1 --arg2 --arg3

We can do the same for test

$ cabal test sometest -- --arg1 --arg2 --arg3

I don't think they go through splitArgs and also are not quoted like with --test-options and in general behave like how you'd expect them to. Initially, we can have both --test-options and this way of passing the args

[Mikolaj]

Oh, yes. In fact, the -- idiom is as common in the wider community as the other one. Similarly, we'd need to make sure we are not creating a caricature of the -- convention, but have a reasonably standard way of separating, quoting, etc. I'd say, it's better to have two ways of doing things than zero ways, so I'd finish and merge this PR before tackling the other.

I think -- is superior when it's obvious which exe you have in mind (e.g., the exe you create and/or pass control to for a long time), while --prog-options is better when you can potentially have many exes to pass parameters to. I think in cabal run and cabal test the main exe is obvious enough, while in cabal build the main exe can be ghc, gcc, etc. I wonder, perhaps cabal test was envisioned to be more general and, e.g., call many test programs in turn. There are potentially more test stanza types than exitcode-stdio-1.0, after all, and there can be many test components that could be selected with globs perhaps. But we are definitely more modest in our plans, currently, so -- makes sense.

[Mikolaj]

Actually, now that I think about it, cabal test already runs all test executables in turn, doesn't it? So to which executable the -- arguments are meant to apply? What if one wants to give different arguments to different test executables?

[pranaysashank]

What does --test-options do?

[Mikolaj]

What does --test-options do?

Indeed. It probably gives the arguments to all text exes, but I haven't checked. We don't yet have --test-foo-option that would let us give different arguments to different tests. Perhaps we should extend --test-options so, but let's not bother until users demand it and are ready to test it in practice.

[ulysses4ever]

This has been discussed (at least) in #5416. The outcome was, in my interpretation, is that there's already --test-option that suffers the same, yet being usable. So it won't hurt (and probably benefit several people who keep opening issues like that) to have -- working for tests even with this concern not addressed.

[Mikolaj]

Right. So I think we need both. -- for comfort, --test-options for backward compatibility and future extensibility to --test-foo-option.

[ulysses4ever]

I created a simple project with a test suite, which prints out what it gets as the arguments:

• splitArgsTest.tar.gz

There are good news and bad news. Let's start with bad:

❯ $cabal-pr8093 test sometest --test-options "-p findRoot'"
...
Test suite sometest: RUNNING...
-p
findRoot
Test suite sometest: PASS

So, the trailing quote ( ') disappears. The current cabal does preserve it of course.

Good news:

❯ $cabal-pr8093 test sometest --test-options "-p \"findRoot'\""
Test suite sometest: RUNNING...
-p
findRoot'
Test suite sometest: PASS

Note you can try it yourself with cabal built in the CI for this PR:

• Linux
• Mac
• Windows

---

RE comparison to Unix tools, e.g. echo. I'm not sure it's very easy to compare. E.g. is

cabal test sometest --test-options="don't"

(recall, with the patch it kills the quote and you get dont) considered an analog of the first or second echo below

$ echo don't
> ^C
$ echo "don't"
don't

I understand you argue its the former (it's as if the test gets a "naked" don't as the input). I'm afraid it may be surprising for someone who didn't think it through. By the way, if we really want this patch and follow the former, we should fail rather than silently eat the quote.

---

As the bottomline, I'm torn... Perhaps, eating quotes will break someone's testing code. Whereas, the '-inside-" vs. "-inside-' difference (the original motivation in 8090) could perhaps be just documented. And if we do pull out the proper behavior for --, this will become even less notable. So. would it be worth breaking someone's code?

[pranaysashank]

As the bottomline, I'm torn... Perhaps, eating quotes will break someone's testing code. Whereas, the '-inside-" vs. "-inside-' difference (the original motivation in 8090) could perhaps be just documented. And if we do pull out the proper behavior for --, this will become even less notable. So. would it be worth breaking someone's code?

I am inclined towards not introducing the breaking change in this PR, and just document this in the cabal test --help options

[Mikolaj]

This is a very in-depth discussion. I totally defer to your judgement @pranaysashank and @ulysses4ever.

If you endeavour to fix the help texts, pretty please include also the \"..\" quotes. Even if it's a standard unix trick to you, I barely ever encounter it and it's very useful in this context. Which reminds me that currently things like colons are invalid inside '-p prefix "..."', but valid with the \" idiom. So there are more problems there that just inflexible order of quotes. Poor users deserve some handholding at least.

[Kleidukos]

Marking this PR as draft 🙂