Consistently use the Cabal version picked by the dependency solver. #3723

grayjay · 2016-08-28T03:19:37Z

Previously, cabal always chose a Cabal library version to use for a package's
setup script that didn't take setup-depends into account (cabalLibVersion).
cabalLibVersion depended on the cabal-version field, the installed versions of
Cabal, etc., and it was used when setup dependencies were not chosen by the
dependency solver. When a package had a setup-depends, cabalLibVersion was later
ignored in favor of the version chosen by the dependency solver. However,
calculating the variable caused an error when the only suitable Cabal version
was installed during the same install command (issue #3436). cabalLibVersion was
also used to filter the configure flags (issue #3433).

This commit sets cabalLibVersion to the version chosen by the dependency solver,
when possible.

I added an integration test for issue #3436 in the second commit. I also added two more custom-setup tests in the third commit, but they passed before this change.

EDIT: This fixes #3433, #3436, and #3475.

Previously, cabal always chose a Cabal library version to use for a package's setup script that didn't take setup-depends into account (cabalLibVersion). cabalLibVersion depended on the cabal-version field, the installed versions of Cabal, etc., and it was used when setup dependencies were not chosen by the dependency solver. When a package had a setup-depends, cabalLibVersion was later ignored in favor of the version chosen by the dependency solver. However, calculating the variable caused an error when the only suitable Cabal version was installed during the same install command (issue haskell#3436). cabalLibVersion was also used to filter the configure flags (issue haskell#3433). This commit sets cabalLibVersion to the version chosen by the dependency solver, when possible.

mention-bot · 2016-08-28T03:19:38Z

@grayjay, thanks for your PR! By analyzing the annotation information on this pull request, we identified @dcoutts, @23Skidoo and @ezyang to be potential reviewers

…abal.

ezyang · 2016-08-28T05:33:37Z

cabal-install/Distribution/Client/SetupWrapper.hs

                             ,SetupScriptOptions)
  cabalLibVersionToUse =
-    case useCabalSpecVersion options of
-      Just version -> do
+    case find (hasCabal . snd) (useDependencies options) of


I'd appreciate a comment laying out the logic here, and a reference to the issues that arise when you don't do it correctly. (Yes you can blame, but right now it's not even obvious something nontrivial is going on here.)

Doesn't the solver pass the Cabal version to us as useCabalVersion? Can we just use that instead?

@23Skidoo The comment over the useCabalVersion field says that it should be used when useDependenciesExclusive is not set, so it isn't always set by the dependency solver. I used useDependencies because SetupWrapper already uses it to check whether the solver has picked a Cabal version:

cabal/cabal-install/Distribution/Client/SetupWrapper.hs

Line 629 in c4fc3bd

if any hasCabal (useDependencies options')

.

@grayjay I think the comment refers to the fact that if useDependenciesExclusive is set, then we should just use those dependencies exclusively, including the dependency on Cabal. If you look at the code, in both places we set useDependencies, we also set useCabalVersion.

I'm just a bit worried that the already complicated logic in this file is becoming more complicated, so I'm thinking about how we can make it simpler.

The code still needs to determine whether the solver chose the Cabal version, though, which would be hard to determine from useCabalVersion. If the solver chose a Cabal version then we could use useCabalVersion. But I didn't want to change the behavior in the case that the solver didn't choose any setup dependencies or chose setup dependencies that didn't include Cabal.

OK, let's merge your patch as-is and refactor the logic some other time.

@ezyang I pushed a new commit with comments.

@23Skidoo I agree that the logic is becoming complicated. We could separate out the old and new code paths, as suggested here #3094 (comment). Then it would be easier to see which SetupScriptOptions apply in each case.

ezyang · 2016-08-28T05:34:09Z

LGTM.

23Skidoo · 2016-08-28T11:10:09Z

/cc @edsko

I think this should also go into the 1.24 branch.

ezyang · 2016-08-29T05:11:53Z

Thanks @grayjay, the docs are much appreciated.

Reading them, I wonder: how does the solver decide which version of Cabal to pick? Clearly it tries to respect setup dependencies, but do any of the other parameters feed into it? Based on what I've seen, I don't think so, but I could be wrong?

grayjay · 2016-08-30T05:30:52Z

@ezyang AFAIK, the solver only considers the setup dependencies. It ignores cabal-version, so it is possible for the solver to choose a version of Cabal that later refuses to configure the package. We should probably add a check for packages with a setup-depends and cabal-version that are inconsistent, or take the intersection of the ranges before dependency solving.

ezyang · 2016-08-30T05:33:47Z

Yeah, it seems to me applying cabal-version when solving for Custom seems like the right thing to do. Doesn't have to be for this patch but something to keep in mind.

ezyang · 2016-09-01T02:47:09Z

OK, so I was trying to document cabal-lib-version and I realized that I have no idea what this is supposed to do. What are the intended semantics of this field; e.g., when would I put it in my cabal.project?

grayjay · 2016-09-01T03:04:54Z

@ezyang Do you mean cabal-version? It is the version of the Cabal specification used by the .cabal file, and it's documented in https://github.com/haskell/cabal/blob/master/Cabal/doc/developing-packages.markdown.

ezyang · 2016-09-01T03:07:19Z

Nope, I'm referring to the --cabal-lib-version flag, which gets set to configCabalVersion

23Skidoo · 2016-09-01T04:53:51Z

IIRC, --cabal-lib-version is aimed at developers and is used to force setup scripts to be compiled against a different Cabal version than the one that'd be picked up automatically. So you can test e.g. that cabal-install doesn't pass unsupported flags to scripts compiled against old versions of Cabal.

ezyang · 2016-09-01T05:03:14Z

OK, but that doesn't answer all my questions:

Does it apply only if I have Custom, or for Simple as well?
If there is a setup-depends field, does it override the setup depends field? Or not?
Is there a way to override this for just one package? At the moment, it's a global field.

Does anyone actually use this? Maybe we should drop it.

grayjay · 2016-09-01T05:07:14Z

Yeah, it seems to me applying cabal-version when solving for Custom seems like the right thing to do. Doesn't have to be for this patch but something to keep in mind.

I opened an issue, #3751.

Should I merge this and create a PR for 1.24?

23Skidoo · 2016-09-01T05:26:40Z

Does it apply only if I have Custom, or for Simple as well?

I think it does, in that case it forces the external setup method.

If there is a setup-depends field, does it override the setup depends field? Or not?

It predates the introduction of setup-depends, so I'm not sure what actually happens. I'd expect an error.

Is there a way to override this for just one package? At the moment, it's a global field.

I don't think so. Normally you use it in a single package directory anyway.

Does anyone actually use this? Maybe we should drop it.

I haven't used it much lately. I think that one can now use a package with a setup dependency on Cabal for the same purposes.

grayjay · 2016-09-06T01:24:05Z

@ezyang, @23Skidoo Thanks for the code review!

Cherry-pick #3688 and #3723 onto 1.24

grayjay added 2 commits August 27, 2016 19:41

Test for issue haskell#3436.

9ec4028

grayjay added the Cabal: stanza/custom-setup label Aug 28, 2016

grayjay mentioned this pull request Aug 28, 2016

Have to install twice package with 'custom-setup' stanza #3436

Closed

Test configuring a package with a custom-setup that doesn't include C…

4eb8e4f

…abal.

grayjay force-pushed the issue-3436 branch from a539e49 to 4eb8e4f Compare August 28, 2016 03:32

ezyang reviewed Aug 28, 2016
View reviewed changes

23Skidoo added this to the Cabal 1.24.0.1 milestone Aug 28, 2016

Document choosing the Cabal version for a setup script.

a94764e

grayjay mentioned this pull request Sep 1, 2016

cabal ignores the 'cabal-version' field when solving for setup dependencies #3751

Open

grayjay mentioned this pull request Sep 4, 2016

SetupWrapper: separate determining and running Setup #3762

Merged

ezyang merged commit 3a96032 into haskell:master Sep 6, 2016

grayjay deleted the issue-3436 branch September 6, 2016 01:24

grayjay self-assigned this Sep 7, 2016

grayjay mentioned this pull request Sep 8, 2016

Cherry-pick #3688 and #3723 onto 1.24 #3805

Merged

grayjay removed their assignment Sep 8, 2016

ezyang added a commit that referenced this pull request Sep 8, 2016

Merge pull request #3805 from grayjay/issue-3436-1.24

e374b6d

Cherry-pick #3688 and #3723 onto 1.24

Consistently use the Cabal version picked by the dependency solver. #3723

Consistently use the Cabal version picked by the dependency solver. #3723

Uh oh!

Conversation

grayjay commented Aug 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Aug 28, 2016

Uh oh!

ezyang Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

23Skidoo Aug 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grayjay Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

23Skidoo Aug 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

23Skidoo Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

grayjay Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

23Skidoo Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

grayjay Aug 29, 2016

Choose a reason for hiding this comment

Uh oh!

grayjay Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

ezyang commented Aug 28, 2016

Uh oh!

23Skidoo commented Aug 28, 2016

Uh oh!

ezyang commented Aug 29, 2016

Uh oh!

grayjay commented Aug 30, 2016

Uh oh!

ezyang commented Aug 30, 2016

Uh oh!

ezyang commented Sep 1, 2016

Uh oh!

grayjay commented Sep 1, 2016

Uh oh!

ezyang commented Sep 1, 2016

Uh oh!

23Skidoo commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezyang commented Sep 1, 2016

Uh oh!

grayjay commented Sep 1, 2016

Uh oh!

23Skidoo commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grayjay commented Sep 6, 2016

Uh oh!

Uh oh!

grayjay commented Aug 28, 2016 •

edited

Loading

23Skidoo Aug 28, 2016 •

edited

Loading

23Skidoo Aug 28, 2016 •

edited

Loading

23Skidoo commented Sep 1, 2016 •

edited

Loading

23Skidoo commented Sep 1, 2016 •

edited

Loading