Escaping of ghc-options with spaces

[snoyberg]

Consider the following cabal file:

name:           foo
version:        0.1.0.0
build-type:     Simple
cabal-version:  >= 1.10

executable foo
  main-is: Main.hs
  build-depends: base
  default-language: Haskell2010
  ghc-options: -threaded -O2 -rtsopts "-with-rtsopts=-N -T"

Using hpack-convert, I end up with:

name: foo
version: '0.1.0.0'
license: UnspecifiedLicense
ghc-options:
- -threaded
- -O2
- -rtsopts
- -with-rtsopts=-N -T
dependencies:
- base
executables:
  foo:
    main: Main.hs

Which results in the following in the generated cabal file:

  ghc-options: -threaded -O2 -rtsopts -with-rtsopts=-N -T

Unfortunately, this does not build:

ghc: unrecognised flag: -T
did you mean one of:
  -F
  -v
  -j

I can work around this by putting the following in my package.yaml instead of the list of options:

ghc-options: -threaded -O2 -rtsopts "-with-rtsopts=-N -T"

However, it would be nice to keep the list approach, and have hpack detect when there's a space in an option and automatically wrap it in quotes.

[tfausak]

Sounds a lot like #182. I think fixing this in hpack (#238) makes more sense than requiring people to muck around with YAML syntax and GHC argument parsing.

[sol]

@snoyberg thanks for opening this issue.

I think fixing this in hpack (#238) makes more sense than requiring people to muck around with YAML syntax and GHC argument parsing.

Yes, I agree. If there is a way to address this in hpack I'm all for it. However, just quoting everything may do more harm than good. So let's first collect all the facts.

Specifically:

1. Is -with-rtsopts the only affected GHC option?

2. Currently, the following is valid in package.yaml:

   ghc-options: -Wall -Werror

   If we quote all options that contain spaces the above will become invalid.

3. We could consider to only quote options that were given as a YAML list, e.g.

   ghc-options:
     - -with-rtsopts=-N -T

   we would rendered as ghc-options: "-with-rtsopts=-N -T", while

   ghc-options: -Wall -Werror

   we would continue to render as ghc-options: -Wall -Werror.

   This is easy to do, however, we would loose the property that a scalar and a singleton list have the same semantics.

4. Could Add hash option supports #217 help with this? Say we only quote when option were given as a hash, e.g.

   ghc-options:
     with-rtsopts: -N -T

5. If this whole story is really only relevant for -with-rtsopts, we could consider to introduce a separate field for that, e.g.

   ghc-options: -Wall
   rts-options: -N -T

   would render to ghc-options: -Wall "-with-rtsopts=-N -T".

[snoyberg]

I'm only aware of it affecting with-rtsopts, but I have no idea if that's really the only one. I had been thinking of this going in the direction of (3), with different behavior for scalars and lists. This seems to be a pretty common pattern in config files (Docker does similar things), and even mirrors the behavior of some similar API calls in Haskell (e.g., the difference between proc and shell in the process package).

All this said, I have no strong opinions at all on the outcome here, and definitely don't want to put any pressure on a fix: my current workaround is working fine for me for now.

[tfausak]

1. I've only seen this problem with -with-rtsopts=..., but GHC has so many options it's hard to say. Something like -optF=... could also be a problem?
2. I think the cases of a single string and an array of strings should be handled separately as far as this is concerned. A single string means literally "pass this as-is to GHC". An array means "each of this is an atomic option".
3. Yup, I agree.
4. If hashes are implemented for this, I imagine they would behave a lot like lists. It might be a little tricky because Q: How to pass "-with-rtsopts=-N -I0 -qb -qg" to ghc-options? #182 notes that "-with-rtsopts=-N -T" is fine but -with-rtsopts="-N -T" doesn't work. So only quoting the value is a no-go for this particular case.
5. I'm weakly -1 on adding a custom options for -with-rtsopts.

Semi-related: I've mostly given up on specify -with-rtsopts in my package descriptions. I set -threaded and -rtsopts; if users want to set +RTS -N, they can, but I'll stick with GHC's defaults.

[sol]

As for (3), seems that this won't work neither. Let's consider:

ghc-options:
  - -main-is Foo.run

If we now quote this and generate ghc-options: "-main-is Foo.run" that won't be accepted by GHC.

At this point I'm tempted to consider special casing -with-rtsopt, either with magic or by emitting a warning if we see -with-rtsopt unquoted.

[sol]

As I understand it, all of the following are valid ways of specifying -with-rtsopts in a cabal file:

a) ghc-options: "-with-rtsopts -N -T"
b) ghc-options: "-with-rtsopts=-N -T"
c) ghc-options: -with-rtsopts "-N -T"
d) ghc-options: -with-rtsopts=-N
e) ghc-options: -with-rtsopts -N

In package.yaml c), d) and e) work as well. a) and b) require double quoting.

ghc-options: "-with-rtsopts -N -T" # invalid
ghc-options: "-with-rtsopts=-N -T" # invalid

# needs double quoting
ghc-options: '"-with-rtsopts -N -T"' # valid
ghc-options: '"-with-rtsopts=-N -T"' # valid

One form of magic we could introduce is

When a GHC option starts with -with-rtsopts we quote it when rendering, e.g.

ghc-options: -with-rtsopts -N -T # valid

Which has the unfortunate downside that some currently valid package specifications will become invalid, e.g.

ghc-options: -with-rtsopts -N -Wall # currently valid, but invalid if we introduce that magic behavior

We could try to mitigate this by only applying magic when GHC options are given as a list, e.g.

ghc-options: -with-rtsopts -N -T # invalid

ghc-options:
 - -with-rtsopts -N -T # valid

ghc-options: -with-rtsopts -N -Wall # valid

ghc-options:
 - -with-rtsopts -N -Wall # invalid

Not very excited either way! Opinions?

[tfausak]

Hmm, we're in a weird situation here because the YAML array syntax implies "each of these is exactly one argument" but stuff like -main-is Main.hs breaks that. If we really followed that guideline, we'd end up with this:

ghc-options:
  - -main-is
  - Main.hs
  - -with-rtsopts
  - -N -T

And then quoting arguments with spaces in them would be no problem.

I'm not sure what the right way forward is. At this point, I don't mind quoting and dealing with YAML syntax oddities. I think I might prefer it to magic, but I don't hold that opinion very strongly.

[quasicomputational]

I'd like to +1 predictability and going with YAML syntax here. Too many corner cases otherwise and I'm sure one would turn around and bite us eventually. One direct change to make here would be to add a note to the docs about this case, and how to properly escape the input (which, unless I'm misreading, can be achieved today, even if it's ugly?).

Maybe the more hpackish way forward would be to reduce the need to directly deal with options-as-strings at all by adding structure? Something like this:

ghc-options:
  main-is: Main.hs
  threaded: true
  optimisation: 2
  warnings:
    - all
    - error
    - no-name-shadowing
  rts-options:
    threads: true
    collect-statistics: true

This is a bit like #217, but hpack will learn about all these weird and wonderful GHC options and how to Do The Right Thing for them, rather than being a generic mechanism. ghc-options should still be specifyable as a list of strings as an escape hatch, too.

[sol]

Ok, to sum things up, as I understand it non of us is excited about introducing any magic behavior here, so let's exclude that. What we are left with are the following options:

1. Close as wontfix
2. Add a dedicated rts-options field
3. Add hash syntax for GHC options

Even though (3) is nice, I would want to exclude it as well for the following reasons:

• first and foremost, it's a non-trivial effort to implement and maintain; even if somebody else does the initial implementation I'm not ready to take responsibility to maintain it in the future; if there is strong demand we could revisit once we are at zero open issues and PRs
• and secondly a more minor point, it does not allow to copy & paste options from the command line into package.yaml and vice versa

I'm ok with (1) but the fact that we already have more than one issue on this clearly indicates that this is a stumbling block. So maybe we should do something about it.

(2) would look like this, we add support for an rts-options field which we know how to quote correctly. In addition we check if ghc-options contains the substring -with-rtsopts and if so, we aggressively warn and suggest to the user to specify them as rts-options: ... instead. A warning could look like this:

WARNING: The field "ghc-options" specifies "-with-rtsopts".  This is fragile and hence not
recommended. Please consider specifying them in the field "rts-options" instead.

For more information see: <URL to the relevant section in the README>

To silence this warning do ....

Concerns regarding (2):

• Is it too invasive, especially for existing package configurations?
• In theory, -with-rtsopts could be part of some valid GHC option (e.g. it could be the name of a custom preprocessor or unlit). If that happens the user would get a warning even though nothing is wrong. I think it's very very unlikely that this will lead to issues in practice, and we can mitigate it further by taking more context into account when doing the substring matching (say make sure that we only have one of , ", = at the word boundaries). In addition, I guess some way to silence the warning would be a good idea. But still, I wanted to mention it.

Before making a final call on either (1) or (2) I would be deeply grateful for an other round of feedback from you guys.

Thanks and sorry for the lengthiness!

[tfausak]

I am happy with how things are now, but I think option 2 (a dedicated field for RTS options) is the best way forward to prevent confusion. It would give first class support to RTS options and keep ghc-options as an escape hatch. My only concern is that this might lead to an explosion of new fields (as @quasicomputational hinted at). That's not necessarily a problem, though. It's just more stuff to code, test, document, and maintain.