parsec for cabal-install

[phadej]

No description provided.

[mention-bot]

@phadej, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @owst and @23Skidoo to be potential reviewers.

[ezyang]

The cabal-install patch doesn't actually work, right? Because the parsec flag doesn't control whether the Cabal parsec flag is toggled, and you really need it to be True. But there's no way to declare that you need a flag toggled on a library.

Maybe best to bite the bullet and have Cabal unconditionally build the Parsec bits.

[ezyang]

Clients tend to appreciate it when you explain why the new name is "better".

[phadej]

Cabal cannot build parsec unconditionally, before GHC build system has parsec etc. set up.

[phadej]

Can someone advance: haskell/hackage-security#179
I don't want to introduce CPP just to make tests compile warning free with different versions of optparse-applicative. Ping @hvr, @dcoutts, @edsko

[ezyang]

@phadej OK, but we could plan on setting up GHC's build system to build parsec for 8.2. It will require some work but it should be within the realm of what we know how to do. So what's the plan for fixing this then?

[phadej]

@ezyang To my understanding parsec-parser was planned for Cabal 2.2, which going to be bundled with GHC 8.4. Thus I wasn't in a hurry to push this forward on GHC side, I don't think it's a good idea to introduce new build dependencies at this stage. (GHC 8.2 feature freeze is end of January, i.e. this month).

[phadej]

So, can I merge this, or does @ezyang object?

[23Skidoo]

OK to merge IMO.

[ezyang]

@phadej I reread the conversation, and I still don't see why cabal-install is guaranteed to get a version of Cabal with parsec enabled. To trigger the problem I'm theorizing about, try running a build with --constraint "Cabal -parsec" --constraint "cabal-install +parsec". As long as this problem exists, I don't think we should merge this patch.

Here's an alternative: export a module which is the same between the Parsec/non-enabled Parsec versions, and have cabal-install just import that. Then we are indifferent to flag selection.

[23Skidoo]

Here's an alternative: export a module which is the same between the Parsec/non-enabled Parsec versions, and have cabal-install just import that. Then we are indifferent to flag selection.

I think that this means making Cabal always depend on Parsec, which is what we want to avoid for now.

[ezyang]

@23Skidoo The type signature of readGenericPackageDescription is Verbosity -> FilePath -> IO GenericPackageDescription, so if it is CPP'ed between reguilar and Parsec implementations that won't leak use of Parsec.

[23Skidoo]

But how will you make sure that cabal-install built with -fparsec actually uses Parsec? IMO it's better for it to fail loudly when built against wrong Cabal. This stuff has experimental status, so I think it's OK in this case.

[ezyang]

Simple: you delete the parsec flag from cabal-install, and say "if you want to have cabal-install that parses package descriptions with Parsec, build it against a +parsec Cabal"

[23Skidoo]

I'm okay with that.

[phadej]

The problem is that I'd want (soon enough) to use parsec machinery to e.g. parse cabal.project. Than I cannot pretend parsec doesn't exist.

[23Skidoo]

One other thing we can do here is branch 2.0 (given that GHC 8.2 will be out ~next month, I think it's time) and enable Parsec parser unconditionally in master.

[Ericson2314]

Maybe we add a feature to require flags in libaries, and use it in post 2.0 cabal-install? [As per rust-lang/rfcs#1787 (comment) only manual flags should be allowed to change the interface.] IIRC cabal-install doesn't need to be built with old versions like Cabal, so this is OK?

[23Skidoo]

@Ericson2314 See #2821 for previous discussion of that idea. Also, we can't simply set cabal-version: >= 2.2 in cabal-install.cabal in the 2.2 release, as then cabal install cabal-install will stop working with the previous version.

[Ericson2314]

@23Skidoo Err i mean squeeze this feature into 2.0, and then use it for 2.2.

[phadej]

What if we remove flag parsec from cabal-install before making release (i.e. in 2.0 branch). Then master can be developed normally, and 2.0 cannot be built requiring Cabal with parsec enabled.

Of course you can do stupid things like --cpp-options=-DCABAL_PARSEC, but definitely you cannot do that by accident.

[ezyang]

OK, if the motivation is starting work on cabal.project Parsec parsing, I accept that adding another flag to cabal-install is expedient. But we must clearly describe (a) how to use the flag correctly (you must also pass +parsec to Cabal), (b) what the eventual plan is (Cabal library will have Parsec unconditionally, so we can remove the flags) and (c) an issue tracking the removal of the flag.

[phadej]

a) amended flag description
b) c) #4245

[ezyang]

AppVeyor Werror fail:

Distribution\Client\Outdated.hs:137:11: warning: [-Wdeprecations]
    In the use of `readPackageDescription'
    (imported from Distribution.PackageDescription.Parse):
    Deprecated: "Use readGenericPackageDescription, old name is missleading."
<no location info>: error: 
Failing due to -Werror.

[phadej]

Hmm... That's introduced by outdated patch. Trivial to fix, but won't until someone says "good to merge after CI is green" or says what's wrong otherwise. (We could start using Githubs reviews, with "accepted" / "changes requested")

[23Skidoo]

From the discussion above it looks like we all agree that this is good to merge.

[phadej]

Travis OS X has a bad day