Makes flags overridable on the command line.

[dalaing]

This is to fix issue #4452, but updating FlagAssignment to be a Map with the appropriate Semigroup and Monoid instance that favours later entries.

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.
• If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

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

I ran variations of cabal act-as-setup -- configure -fdev -f-dev -v and cabal configure -fdev -f-dev -v on a dummy project to test this out. I'm happy to have a go at adding some tests on Monday if that helps - those bonus points are tempting me...

[23Skidoo]

Nice, thanks!

[23Skidoo]

LGTM.

[dalaing]

I just added some things to tidy up some things based on comments in the surrounding code - mostly to do with the duplicates check that is no longer necessary in configuredPackageProblems.

[hvr]

@23Skidoo The reason I originally didn't turn the newtype into a Map was because this would throw the duplicate-flag validation out. Do we really want to fail silently here / be tolerant towards duplicate flags?

[dalaing]

I could change it to a map to (Int, Bool) where the Int counts the number of times that flag was observed and restore the duplicate flags warning via those means.

[23Skidoo]

So that you'd get a warning about -fflag -f-flag? Makes sense.

[dalaing]

I'm not exactly sure if/when you get a warning, but it should at least be compatible with the existing code that checks for duplicate flags.

[gwils]

There is an 'a' both before and after the line break.

[23Skidoo]

Good catch!

[23Skidoo]

LGTM, feel free to merge once this is green.

[dalaing]

I'm going to try bumping the hash of the custom-setup component for the failing test to see if that helps / if the other tests pass after that. I figured I should ask while that is running: is that a valid thing to do for this kind of change?

[23Skidoo]

bumping the hash of the custom-setup component

I'm not quite sure what you have in mind here. Anyway, I think the failing tests were mostly due to #4962.

[dalaing]

In the last run, there was a difference in the custom-setup hash in cabal-testsuite/PackageTests/Regression/T3436/sandbox.out that was breaking things.

[23Skidoo]

I see. It's most likely okay to do this in this case.

[dalaing]

This is now failing in the print / parse round-trip property based tests.

I can see two ways to deal with this: I can write something to reset the count of duplicates in the various data structures that contain FlagAssignments, or I can write an instance Eq FlagAssignment that ignores the count of duplicates.

I'm inclined to go for the latter, because with have findDuplicateAssignments for when the counts matter, and other than where that code is used I don't think the counts will come up that often.

[dalaing]

I've put up custom instances of Eq and Ord as a first pass for this. I can write a version of the Binary instance as well if that seems helpful / needed.

[23Skidoo]

The sole remaining test failure is a timeout. Restarted.

[hvr]

@dalaing fwiw, there was also the idea to make make FlagAssignment take a type-parameter; so we could have

• FlagAssignment Bool
• FlagAssignment BoolSet

data BoolSet = BoolSet !Bool !Int

or some other variant

that way we'd be able to have two types, one where the uniqueness invariant is guaranteed and it has a heap-friendly representation (since Bool's values are singletons on the heap; and also because we don't lose anything by using a type-var, since the underlying Map also has a type-var for the value)

while the BoolSet encodes our input w/ annotations about ambiguity/conflicts. and we validate the invariant when flattening FlagAssignment BoolSet to FlagAssignment Bool

that way we also don't have to introduce Eq instances which violate leibniz equality...

[dalaing]

I think something is actually a bit wrong with this at the moment.

We have the Ints in there so that we can use them to detect duplicates in cabal-install/Distribution/Client/Dependency.hs:configuredPackageProblems, which is used in resolveDependencies.

That seems to have been not picking up on duplicates since before this change. I've been trying a few things and I'm unable to trigger a duplicate flag error, and I'm having trouble tracking down why. Maybe the FlagAssignments are round tripping through a list or getting otherwise partly de-duplicated already?

If that check is no longer needed (rather than just being temporarily broken), then maybe we don't need the Ints in the FlagAssignments anymore.

[23Skidoo]

I'd prefer to fix the duplicate check if possible.

[23Skidoo]

Looks like the duplicates check in D.C.Dependency is just a sanity check for the solver's output. I'm actually starting to think that with this change it could be omitted, since the result should be now correct by construction. @grayjay, do you agree?

But we can add checks for duplicates at the locations where FlagAssignment is constructed, so that -ffoo -ffoo would generate a warning.

[23Skidoo]

Merged manually (see 4e4180b). Thanks!

[23Skidoo]

But we can add checks for duplicates at the locations where FlagAssignment is constructed, so that -ffoo -ffoo would generate a warning.

Looks like doing this is somewhat harder than I expected, since it's normally constructed directly via parsecFlagAssignment. I tried emitting a PWarning, but it looks like it just gets ignored by parsecToReadE.

[grayjay]

Looks like the duplicates check in D.C.Dependency is just a sanity check for the solver's output. I'm actually starting to think that with this change it could be omitted, since the result should be now correct by construction. @grayjay, do you agree?

Yes, I think that configuredPackageProblems just double checks the solver's output.