Skip to content

Cannot override --flags= on command line #4452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kolmodin opened this issue Apr 17, 2017 · 7 comments
Closed

Cannot override --flags= on command line #4452

kolmodin opened this issue Apr 17, 2017 · 7 comments

Comments

@kolmodin
Copy link
Member

kolmodin commented Apr 17, 2017

WARNING There are updates to this ticket, please read the discussion.

Summary. It's common that the last given value of a flag "wins".

$ cabal configure -fdev -v
...
Flags chosen: dev=True
...

$ cabal configure -f-dev -v
...
Flags chosen: dev=False
....

$ cabal configure -fdev -f-dev -v
...
Flags chosen: dev=True
...

I expected the last line to have set dev=False. According to the documentation it should be dev=False.

Explanation (by @ezyang).

Cabal has quite flexible handling of duplicate instances of arguments when doing command line handling. It does so using monoids, which are types which have a binary "combining" operation defined for them. ConfigFlags, defined in Cabal/Distribution/Simple/Setup.hs, defines the monoid instance for ALL configuration flags, by applying the monoid for each individual field.

Flag assignment is recorded in configConfigurationsFlags, which is defined to be a list. The monoid instance for lists is append. This means that when a flag shows up twice, the lists get appended together; with the example above, you end up with [("dev", True), ("dev", False)].

The bug is in this code Distribution/PackageDescription/Configuration.hs:

    d2c manual n b = case lookup n userflags of
                     Just val -> [val]
                     Nothing
                      | manual -> [b]
                      | otherwise -> [b, not b]

Here, userflags is the list of flag assignments, possibly with duplicates, in the order they occurred on the command line. The problem is that lookup will pick up the FIRST entry, rather than the LAST entry. A better and more efficient strategy is to build a map, preferring the last entry, and then do a lookup on this map. If you do that, the bug will be fixed!

What you will learn. How to build Cabal. How to convert a key-value list of items into a map, and then do a lookup on it. How to do command line arguments with Monoids. How to look through the verbose output of cabal configure to find the info you need.

Going further. This bug doesn't really require a test, but if you want to add one, it should be fairly easy. Follow the instructions in https://github.com/haskell/cabal/tree/master/cabal-testsuite

@kolmodin
Copy link
Member Author

$ cabal --version
cabal-install version 1.25.0.0
compiled using version 1.25.0.0 of the Cabal library 

@23Skidoo
Copy link
Member

Makes sense.

@colinwahl
Copy link
Contributor

colinwahl commented May 8, 2017

Hi everybody. I have made the change to Distribution/PackageDescription/Configuration.hs to use a map instead of a list, but I am having trouble seeing the output

$ cabal configure -f-dev -v
...
Flags chosen: dev=False
....

as shown in the description. Could anyone point me to where I am going wrong?

EDIT: to be more specific. I run cabal configure -f-dev -v with my newly built cabal, but I can't find the Flags chose: dev=False line. I tried grepping for it and that did not work either.

@ezyang
Copy link
Contributor

ezyang commented May 9, 2017

Hi @colinwahl! @krpopo has been looking at this ticket and discovered that there are actually two buggy call sites. With the change I suggested, cabal act-as-setup -- configure -fdev -f-dev -v works correctly (this call bypasses cabal-install and goes directly to Cabal's entry point), but cabal configure does not. In fact, there appears to be another lookup site in cabal-install (in cabal-install/Distribution/Solver/Modular/Preference.hs) which is causing cabal configure to continue to be buggy. Fixing that one should fix all the visible bugs, but it's possible we should refactor FlagAssignment to be a map rather than a list and solve all occurrences of this bug.

@krpopo
Copy link

krpopo commented May 11, 2017

I don't plan to work on this any more

@23Skidoo
Copy link
Member

23Skidoo commented Jun 9, 2017

@VetoPlayer is looking into this one.

dalaing added a commit to dalaing/cabal that referenced this issue Dec 8, 2017
dalaing added a commit to dalaing/cabal that referenced this issue Dec 8, 2017
dalaing added a commit to dalaing/cabal that referenced this issue Dec 9, 2017
dalaing added a commit to dalaing/cabal that referenced this issue Dec 12, 2017
dalaing added a commit to dalaing/cabal that referenced this issue Dec 19, 2017
dalaing added a commit to dalaing/cabal that referenced this issue Dec 20, 2017
23Skidoo pushed a commit that referenced this issue Dec 27, 2017
@23Skidoo
Copy link
Member

Fixed by #4940.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants