Single-quoting ghc-options by space is wrong #1346

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

nh2 commented May 24, 2013

If you put

ghc-options: -with-rtsopts="-A1000 -H1000 -K1000"

into your .cabal file, it will destroy your quoting by single-quoting with every space, calling ghc with

'ghc' ... '-with-rtsopts="-A1000' '-H1000' '-K1000"'

which will fail.

In Distribution.Simple.GHC.buildExe, it will have:

ghcOptExtra = ["-threaded","-with-rtsopts=\"-A1000","-H1000","-K1000\""],

Do you know why we actually single-quote everything in the first place?

Contributor

nh2 commented May 24, 2013

I seem to have a patch for this, will post it soon.

@nh2 nh2 added a commit to nh2/cabal that referenced this pull request May 24, 2013

@nh2 nh2 Parse: Fix parsing of ghc-options and similar. Fixes #1346.
So far,
    ghc-options: -with-rtsopts="-A1000 -H1000 -K1000"
was parsed and run as as
    'ghc' ... '-with-rtsopts="-A1000' '-H1000' '-K1000"'
which will fail.

This is because we *tried* to parse quoted spaces (by leveraging
reads to parse them as Haskell Strings), but only at the beginning
of an option.

This commit fixes it by allowing String literals also in the middle
of options.

Note that because of the fact that we are still using reads,
    ghc-options: -with-rtsopts='-A1000 -H1000 -K1000'
(single quotes) will still fail.

The example from above now parses and runs successfully as
    'ghc' ... '-with-rtsopts="-A1000 -H1000 -K1000"'

This change applies to:
* ghc-options
* cc-options
* cpp-options
* ld-options
e7febd1

@nh2 nh2 added a commit to nh2/cabal that referenced this pull request May 24, 2013

@nh2 nh2 Parse: Fix parsing of ghc-options and similar. Fixes #1346.
So far,
    ghc-options: -with-rtsopts="-A1000 -H1000 -K1000"
was parsed and run as as
    'ghc' ... '-with-rtsopts="-A1000' '-H1000' '-K1000"'
which will fail.

This is because we *tried* to parse quoted spaces (by leveraging
reads to parse them as Haskell Strings), but only at the beginning
of an option.

This commit fixes it by allowing String literals also in the middle
of options.

Note that because of the fact that we are still using reads,
    ghc-options: -with-rtsopts='-A1000 -H1000 -K1000'
(single quotes) will still fail.

The example from above now parses and runs successfully as
    'ghc' ... '-with-rtsopts="-A1000 -H1000 -K1000"'

This change applies to:
* ghc-options
* cc-options
* cpp-options
* ld-options
7c06162

@23Skidoo 23Skidoo and 1 other commented on an outdated diff May 25, 2013

Cabal/Distribution/ParseUtils.hs
@@ -215,11 +215,26 @@ commaListField name showF readF get set =
where
set' xs b = set (get b ++ xs) b
-spaceListField :: String -> (a -> Doc) -> ReadP [a] a
- -> (b -> [a]) -> ([a] -> b -> b) -> FieldDescr b
-spaceListField name showF readF get set =
+-- TODO This fact should go in user-visible documentation.
+
+-- | Parses a single command line argument, like "-O2", seperated from its
@23Skidoo

23Skidoo May 25, 2013

Member

s/seperated/separated/

@nh2

nh2 May 25, 2013

Contributor

Thanks, fixed.

@nh2 nh2 added a commit to nh2/cabal that referenced this pull request May 25, 2013

@nh2 nh2 Parse: Fix parsing of ghc-options and similar. Fixes #1346.
So far,
    ghc-options: -with-rtsopts="-A1000 -H1000 -K1000"
was parsed and run as as
    'ghc' ... '-with-rtsopts="-A1000' '-H1000' '-K1000"'
which will fail.

This is because we *tried* to parse quoted spaces (by leveraging
reads to parse them as Haskell Strings), but only at the beginning
of an option.

This commit fixes it by allowing String literals also in the middle
of options.

Note that because of the fact that we are still using reads,
    ghc-options: -with-rtsopts='-A1000 -H1000 -K1000'
(single quotes) will still fail.

The example from above now parses and runs successfully as
    'ghc' ... '-with-rtsopts="-A1000 -H1000 -K1000"'

This change applies to:
* ghc-options
* cc-options
* cpp-options
* ld-options
011dce4
Contributor

nh2 commented May 25, 2013

I had to make a small adjustment related to leading/trailing whitespace being consumed (ambiguous parses). I made it more clear in the comments what consumes how much whitespace, and now cabal list parses all of hackage again.

@nh2 nh2 added a commit to nh2/cabal that referenced this pull request May 25, 2013

@nh2 nh2 Parse: Fix parsing of ghc-options and similar. Fixes #1346.
So far,
    ghc-options: -with-rtsopts="-A1000 -H1000 -K1000"
was parsed and run as as
    'ghc' ... '-with-rtsopts="-A1000' '-H1000' '-K1000"'
which will fail.

This is because we *tried* to parse quoted spaces (by leveraging
reads to parse them as Haskell Strings), but only at the beginning
of an option.

This commit fixes it by allowing String literals also in the middle
of options.

Note that because of the fact that we are still using reads,
    ghc-options: -with-rtsopts='-A1000 -H1000 -K1000'
(single quotes) will still fail.

The example from above now parses and runs successfully as
    'ghc' ... '-with-rtsopts="-A1000 -H1000 -K1000"'

This change applies to:
* ghc-options
* cc-options
* cpp-options
* ld-options
17b3c77

@23Skidoo 23Skidoo and 1 other commented on an outdated diff May 25, 2013

Cabal/Distribution/ParseUtils.hs
+-- the "string literal" looks like a Haskell string literal (reads is used).
+-- Therefore it can also parse
+-- -with-rtsopts="-N4 -A1000 -H1000 -K1000"
+-- as a single command line argument (but not with 'single quotes' since
+-- those are not used in Haskell string literals).
+--
+-- Does not accept leading spaces.
+-- Does accept trailing spaces after Haskell String literals (and consumes
+-- them as such).
+commandLineArgument :: ReadP r String
+-- Try to parse it as a Haskell string first;
+-- * if that works, the whole argument is enclosed in quotes and we don't want
+-- to have them as part of the returned string,
+-- * otherwise parse it as a convential argument; if quotes are found inside,
+-- they become part of the returned string (using show).
+commandLineArgument = parseHaskellString <++ argumentWithPotentialQutesInside
@23Skidoo

23Skidoo May 25, 2013

Member

s/Qutes/Quotes/

@nh2

nh2 May 26, 2013

Contributor

Done.

@23Skidoo 23Skidoo and 1 other commented on an outdated diff May 25, 2013

Cabal/Distribution/ParseUtils.hs
+-- Try to parse it as a Haskell string first;
+-- * if that works, the whole argument is enclosed in quotes and we don't want
+-- to have them as part of the returned string,
+-- * otherwise parse it as a convential argument; if quotes are found inside,
+-- they become part of the returned string (using show).
+commandLineArgument = parseHaskellString <++ argumentWithPotentialQutesInside
+ where
+ literalHaskellString = show `fmap` parseHaskellString
+ argumentWithPotentialQutesInside =
+ concat `fmap` many1 (literalHaskellString <++ singleNonSpace)
+
+-- | Parses a single non-space character and returns it as a string.
+singleNonSpace :: ReadP r String
+singleNonSpace = (:[]) `fmap` satisfy (not . isSpace)
+
+externalOptsField :: String -> (String -> Doc)
@23Skidoo

23Skidoo May 25, 2013

Member

Can you also add a Haddock comment for externalOptsField?

@nh2

nh2 May 26, 2013

Contributor

Done. Thanks for this one especially, the haddoc of optsField was slightly wrong, mentioning cpp-options although that should be here.

@23Skidoo 23Skidoo and 1 other commented on an outdated diff May 25, 2013

Cabal/Distribution/ParseUtils.hs
+-- environment by a space.
+-- Can handle arguments which with spaces inside of quotes, as long as
+-- the "string literal" looks like a Haskell string literal (reads is used).
+-- Therefore it can also parse
+-- -with-rtsopts="-N4 -A1000 -H1000 -K1000"
+-- as a single command line argument (but not with 'single quotes' since
+-- those are not used in Haskell string literals).
+--
+-- Does not accept leading spaces.
+-- Does accept trailing spaces after Haskell String literals (and consumes
+-- them as such).
+commandLineArgument :: ReadP r String
+-- Try to parse it as a Haskell string first;
+-- * if that works, the whole argument is enclosed in quotes and we don't want
+-- to have them as part of the returned string,
+-- * otherwise parse it as a convential argument; if quotes are found inside,
@23Skidoo

23Skidoo May 25, 2013

Member

s/convential/conventional/

@nh2

nh2 May 26, 2013

Contributor

Done.

@23Skidoo 23Skidoo and 1 other commented on an outdated diff May 25, 2013

Cabal/Distribution/ParseUtils.hs
@@ -215,11 +215,41 @@ commaListField name showF readF get set =
where
set' xs b = set (get b ++ xs) b
-spaceListField :: String -> (a -> Doc) -> ReadP [a] a
- -> (b -> [a]) -> ([a] -> b -> b) -> FieldDescr b
-spaceListField name showF readF get set =
+-- TODO This fact should go in user-visible documentation.
+
+-- | Parses a single command line argument, like "-O2", separated from its
+-- environment by a space.
+-- Can handle arguments which with spaces inside of quotes, as long as
@23Skidoo

23Skidoo May 25, 2013

Member

which with spaces inside of quotes

"which have spaces inside of quotes" maybe?

@nh2

nh2 May 26, 2013

Contributor

Done.

Member

23Skidoo commented May 25, 2013

@dcoutts

Can you take a look at this?

nh2 added some commits May 24, 2013

@nh2 nh2 Parse: Fix parsing of ghc-options and similar. Fixes #1346.
So far,
    ghc-options: -with-rtsopts="-A1000 -H1000 -K1000"
was parsed and run as as
    'ghc' ... '-with-rtsopts="-A1000' '-H1000' '-K1000"'
which will fail.

This is because we *tried* to parse quoted spaces (by leveraging
reads to parse them as Haskell Strings), but only at the beginning
of an option.

This commit fixes it by allowing String literals also in the middle
of options.

Note that because of the fact that we are still using reads,
    ghc-options: -with-rtsopts='-A1000 -H1000 -K1000'
(single quotes) will still fail.

The example from above now parses and runs successfully as
    'ghc' ... '-with-rtsopts="-A1000 -H1000 -K1000"'

This change applies to:
* ghc-options
* cc-options
* cpp-options
* ld-options
14464de
@nh2 nh2 Parse: Don't pass on *-options with full quotes if the whole option
is enclosed in a quote
86e8beb
Member

dcoutts commented May 27, 2013

I've not looked at the patches yet, but let me start by saying that I'm not convinced that making the parsing less regular is a good idea and will lead to less confusion. The cabal spec is quite clear that tokens in fields like ghc-options can be separated by spaces or can use Haskell string syntax. It is not unix shell syntax.

That means if you want to pass a flag like: -with-rtsopts="-A1000 -H1000 -K1000" then the correct way to escape it is:

ghc-options: "-with-rtsopts=-A1000 -H1000 -K1000"

That will pass the whole thing as one argument to ghc.

This is uniform behaviour across all these kinds of fields.

Member

dcoutts commented May 27, 2013

Note also that the point about single quoting is a misunderstanding. There is no quoting when we call ghc or any other program. The quoting is only for the purposes of displaying in log output. When we actually call programs we pass the list of arguments so there is no ambiguity and no quote escaping or parsing.

Contributor

nh2 commented May 27, 2013

Note also that the point about single quoting is a misunderstanding.

Yes, totally :)

Contributor

nh2 commented Jun 4, 2013

You are right, I didn't understand that correctly. Quotes around the whole thing, ghc-options: "-with-rtsopts=-A1000 -H1000 -K1000", is the way to go and it makes a lot of sense.

nh2 closed this Jun 4, 2013

@debug-ito debug-ito added a commit to debug-ito/hi-hspec that referenced this pull request May 2, 2016

@debug-ito debug-ito .cabal: quotes for ghc-option flags must enclose the whole, like "key…
…=value"

See haskell/cabal#1346
e809e83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment