Fix splitArgs function.

[pranaysashank]

• Closes #8090.

TODO

• [ ] Check compatibility with #6248.

---

Please include the following checklist in your PR:

• [x] Patches conform to the coding conventions.
• [ ] Any changes that could be relevant to users have been recorded in the changelog.
• [x] 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[bot]]

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[bot]]

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[bot]]

rebase

✅ Branch has been successfully rebased

[ulysses4ever]

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

[Mikolaj]

@mergify rebase

[mergify[bot]]

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 :shrug: . 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).