Skip to content
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

Allow using cabal program itself as the external setup method #2633

Merged
merged 1 commit into from Jun 3, 2015

Conversation

tibbe
Copy link
Member

@tibbe tibbe commented Jun 1, 2015

This fixes issues when the version of Cabal that cabal-install was built
against differs from the one registered in the local package DB. Normally
we compile an external setup against the local Cabal library, which could
lead to failures or inconsistent results compared to using the internal
method.

@dcoutts
Copy link
Contributor

dcoutts commented Jun 1, 2015

Just a -Werror issue.

@23Skidoo
Copy link
Member

23Skidoo commented Jun 1, 2015

getExecutablePath is not available on GHC 7.4.2, we'll need to add it to one of the D.C.Compat modules.

@dcoutts
Copy link
Contributor

dcoutts commented Jun 1, 2015

Right, will have to do that.

@dcoutts
Copy link
Contributor

dcoutts commented Jun 1, 2015

BTW, @tibbe and I did this together, so the design has my +1 already. Needs some testing though.

@tibbe
Copy link
Member Author

tibbe commented Jun 1, 2015

I believe I fixed the -Werror issue (and closed to old pull request due to a restriction on not using git push -f on haskell/cabal). Will look into getExecutablePath tonight.

@tibbe
Copy link
Member Author

tibbe commented Jun 1, 2015

I've addressed the backwards compat issue with getExecutablePath. Turn out we already had a compat version for it in the repo.

@dcoutts
Copy link
Contributor

dcoutts commented Jun 2, 2015

I guess we ought to try and reproduce #2438 or #1938 and check this fixes it.

@@ -222,12 +223,12 @@ setupWrapper verbosity options mpkg cmd flags extraArgs = do
--
determineSetupMethod :: SetupScriptOptions -> BuildType -> SetupMethod
determineSetupMethod options buildType'
| forceExternalSetupMethod options = externalSetupMethod
| buildType' == Custom = externalSetupMethod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a comment that explains the logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the logic is a bit subtle. Could put in a comment here based on my explanation here #1938 (comment)

In particular we need to say that forceExternalSetupMethod is really about using an external process, due to parallel build concerns, since that's not totally clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth renaming it then.

@23Skidoo
Copy link
Member

23Skidoo commented Jun 2, 2015

LGTM.

@tibbe
Copy link
Member Author

tibbe commented Jun 2, 2015

I've verified that this fixes #2438 and #1938. I've also added some comments based on @dcoutts explanation.

This fixes issues when the version of Cabal that cabal-install was built
against differs from the one registered in the local package DB. Normally
we compile an external setup against the local Cabal library, which could
lead to failures or inconsistent results compared to using the internal
method.

This fixes haskell#2438 and fixes haskell#1938.
@23Skidoo
Copy link
Member

23Skidoo commented Jun 2, 2015

Let's merge!

@tibbe
Copy link
Member Author

tibbe commented Jun 2, 2015

@dcoutts?

dcoutts added a commit that referenced this pull request Jun 3, 2015
Allow using cabal program itself as the external setup method
@dcoutts dcoutts merged commit 70600e9 into haskell:master Jun 3, 2015
@dcoutts
Copy link
Contributor

dcoutts commented Jun 3, 2015

Yarr!

nomeata added a commit to nomeata/ghc-heap-view that referenced this pull request Oct 13, 2015
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.

None yet

3 participants