Skip to content

Fix for issue #3932. #4051

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

Merged
merged 4 commits into from
Oct 29, 2016
Merged

Fix for issue #3932. #4051

merged 4 commits into from
Oct 29, 2016

Conversation

dagit
Copy link
Collaborator

@dagit dagit commented Oct 28, 2016

For now require Cabal >= 1.20 in all new-builds. In the future we plan to relax
this to only Setup.hs dependencies.

Note for reviewers: I wasn't sure what to do with the min cabal version constraint in defaultSetupDeps. I left it at 1.18, because I wasn't sure if it makes sense to bump it as well. For example, does that code only run for new-builds? If yes, then I think we can get rid of it because it's redundant with the constraint I added in planPackages. If no, then leaving it retains some backwards compatibility.

For now require Cabal >= 1.20 in all new-builds. In the future we plan to relax
this to only Setup.hs dependencies.
@mention-bot
Copy link

@dagit, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @ezyang and @BardurArantsson to be potential reviewers.

@ezyang ezyang added this to the 2.0 milestone Oct 28, 2016
@ezyang
Copy link
Contributor

ezyang commented Oct 28, 2016

Note for reviewers: I wasn't sure what to do with the min cabal version constraint in defaultSetupDeps. I left it at 1.18, because I wasn't sure if it makes sense to bump it as well. For example, does that code only run for new-builds?

Nope, we don't want to bump because the legacy codepath uses it, and there's no reason to force a higher min-bound there (we don't use the buggy feature.)

@ezyang
Copy link
Contributor

ezyang commented Oct 28, 2016

Looks great, let's merge when Travis goes green.

@ezyang ezyang merged commit 80de7ff into haskell:master Oct 29, 2016
@dcoutts
Copy link
Contributor

dcoutts commented Oct 29, 2016

I'm not sure I'm convinced this is the right approach. It adds a constraint on Cabal globally irrespective of whether there's any setup script involved. For example it means nothing can ever build against an old version of the Cabal lib for any purpose, not just setup scripts.

I see that defaultSetupDeps does not quite cover it because it doesn't cover the case of a setup script with explicit deps. I think the better approach would be to handle that case.

@dcoutts
Copy link
Contributor

dcoutts commented Oct 29, 2016

So I've decided to try going along with the constraint approach, see #4058.

@dagit dagit deleted the bugfix/issue3932 branch October 30, 2016 01:13
erikd pushed a commit to erikd/cabal that referenced this pull request Jan 28, 2017
Constrain Cabal >= 1.20 in all new-build install plans. This solves problems where Cabal 1.18 don't have a good enough API to let us handle the new-style store (we need --dependency flags.)

In the future we plan to relax this to only Setup.hs dependencies.

Fixes issue haskell#3932.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants