Fix handling of default setup deps #3437
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commit 2f97657 added default setup dep handling to the existing install command code paths, but unfortunately broke the handling for the new-build code path. It added a call to addDefaultSetupDependencies into the standardInstallPolicy. That interfered with the addDefaultSetupDependencies that ProjectPlanning was already using. So this patch splits a basicInstallPolicy out of standardInstallPolicy, where the basicInstallPolicy is all the old stuff, and the standardInstallPolicy just adds the addDefaultSetupDependencies that the install/fetch/freeze commands need. So then ProjectPlanning uses just the basicInstallPolicy. The 2f97657 commit also added a new and simpler method to determine if a package has had default setup deps added. Previously ProjectPlanning had to use a rather complex method to remember this information. So this patch removes all that and makes use of the new method. To stop this breaking in future the next patch adds integration tests to cover custom setup script handling. This fixed issue haskell#3394.
13 tasks
|
Seems to fail on Travis. |
|
Hmm, I don't see where you are adding default setup deps in |
Ah, thanks, I see now. Looks good. |
|
I've not yet been able to reproduce the test failure for ghc 7.10. |
|
@23Skidoo so this appears to work now. I've just squashed some things, and turned the verbosity back down. So assuming it's green we should be good to merge. |
Covers 3 of the 4 possible cases: 1. explicit custom setup deps 2. custom setup with implicit/default deps 4. non-custom setup using the internal cabal lib version case 3 is a non-custom setup but where we're forced to use an external cabal lib version. This case is hard to test since it only happens when it's a newer (not older) Cabal lib version that the package requires, e.g. a .cabal file that specifies cabal-version: >= 2.0. Also, add a --with-ghc option to the integration test suite, which lets us more easily test with different ghc versions. Also, don't use parallel builds in any of the integration tests, as the self-exec method will not work, and some tests need to install deps for some ghc versions.
|
Merged, thanks! |
ezyang
modified the milestones:
cabal-install 2.0,
2.0 (planned for next feature release)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit 2f97657 added default setup dep handling to the existing install command code paths, but unfortunately broke the handling for the new-build code path. It added a call to addDefaultSetupDependencies into the standardInstallPolicy. That interfered with the addDefaultSetupDependencies that ProjectPlanning was already using.
So this patch splits a basicInstallPolicy out of standardInstallPolicy, where the basicInstallPolicy is all the old stuff, and the standardInstallPolicy just adds the addDefaultSetupDependencies that the install/fetch/freeze commands need. So then ProjectPlanning uses just the basicInstallPolicy.
The 2f97657 commit also added a new and simpler method to determine if a package has had default setup deps added. Previously ProjectPlanning had to use a rather complex method to remember this information. So this patch removes all that and makes use of the new method.
To stop this breaking in future we also add integration tests for setup script handling, covering 3 of the 4 possible cases:
case 4 is a non-custom setup but where we're forced to use an external cabal lib version. This case is hard to test since it only happens when it's a newer (not older) Cabal lib version that the package requires, e.g. a .cabal file that specifies cabal-version: >= 2.0.
This should fix issue #3394.