Skip to content

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

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
Sep 6, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 45 additions & 19 deletions cabal-install/Distribution/Client/SetupWrapper.hs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ import Control.Applicative ( (<$>), (<*>) )
import Data.Monoid ( mempty )
#endif
import Control.Monad ( when, unless )
import Data.List ( foldl1' )
import Data.List ( find, foldl1' )
import Data.Maybe ( fromMaybe, isJust )
import Data.Char ( isSpace )
import Distribution.Client.Compat.ExecutablePath ( getExecutablePath )
Expand Down Expand Up @@ -389,26 +389,45 @@ externalSetupMethod verbosity options pkg bt mkargs = do
Nothing -> getInstalledPackages verbosity
comp (usePackageDB options') conf

cabalLibVersionToUse :: IO (Version, (Maybe ComponentId)
-- Choose the version of Cabal to use if the setup script has a dependency on
-- Cabal, and possibly update the setup script options. The version also
-- determines how to filter the flags to Setup.
--
-- We first check whether the dependency solver has specified a Cabal version.
-- If it has, we use the solver's version without looking at the installed
-- package index (See issue #3436). Otherwise, we pick the Cabal version by
-- checking 'useCabalSpecVersion', then the saved version, and finally the
-- versions available in the index.
--
-- The version chosen here must match the one used in 'compileSetupExecutable'
-- (See issue #3433).
cabalLibVersionToUse :: IO (Version, Maybe ComponentId
,SetupScriptOptions)
cabalLibVersionToUse =
case useCabalSpecVersion options of
Just version -> do
case find (hasCabal . snd) (useDependencies options) of
Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

Copy link
Member

@23Skidoo 23Skidoo Aug 28, 2016

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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:

if any hasCabal (useDependencies options')
.

Copy link
Member

@23Skidoo 23Skidoo Aug 28, 2016

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ezyang I pushed a new commit with comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

Just (unitId, pkgId) -> do
let version = pkgVersion pkgId
updateSetupScript version bt
writeFile setupVersionFile (show version ++ "\n")
return (version, Nothing, options)
Nothing -> do
savedVer <- savedVersion
case savedVer of
Just version | version `withinRange` useCabalVersion options
-> do updateSetupScript version bt
-- Does the previously compiled setup executable still exist
-- and is it up-to date?
useExisting <- canUseExistingSetup version
if useExisting
then return (version, Nothing, options)
else installedVersion
_ -> installedVersion
writeSetupVersionFile version
return (version, Just unitId, options)
Nothing ->
case useCabalSpecVersion options of
Just version -> do
updateSetupScript version bt
writeSetupVersionFile version
return (version, Nothing, options)
Nothing -> do
savedVer <- savedVersion
case savedVer of
Just version | version `withinRange` useCabalVersion options
-> do updateSetupScript version bt
-- Does the previously compiled setup executable still exist
-- and is it up-to date?
useExisting <- canUseExistingSetup version
if useExisting
then return (version, Nothing, options)
else installedVersion
_ -> installedVersion
where
-- This check duplicates the checks in 'getCachedSetupExecutable' /
-- 'compileSetupExecutable'. Unfortunately, we have to perform it twice
Expand All @@ -424,13 +443,20 @@ externalSetupMethod verbosity options pkg bt mkargs = do
(&&) <$> setupProgFile `existsAndIsMoreRecentThan` setupHs
<*> setupProgFile `existsAndIsMoreRecentThan` setupVersionFile

writeSetupVersionFile :: Version -> IO ()
writeSetupVersionFile version =
writeFile setupVersionFile (show version ++ "\n")

hasCabal (PackageIdentifier (PackageName "Cabal") _) = True
hasCabal _ = False

installedVersion :: IO (Version, Maybe InstalledPackageId
,SetupScriptOptions)
installedVersion = do
(comp, conf, options') <- configureCompiler options
(version, mipkgid, options'') <- installedCabalVersion options' comp conf
updateSetupScript version bt
writeFile setupVersionFile (show version ++ "\n")
writeSetupVersionFile version
return (version, mipkgid, options'')

savedVersion :: IO (Maybe Version)
Expand Down
13 changes: 13 additions & 0 deletions cabal-install/cabal-install.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,19 @@ Extra-Source-Files:
-- Do NOT edit this section manually; instead, run the script.
-- BEGIN gen-extra-source-files
tests/IntegrationTests/common.sh
tests/IntegrationTests/custom-setup/Cabal-99998/Cabal.cabal
tests/IntegrationTests/custom-setup/Cabal-99998/CabalMessage.hs
tests/IntegrationTests/custom-setup/Cabal-99999/Cabal.cabal
tests/IntegrationTests/custom-setup/Cabal-99999/CabalMessage.hs
tests/IntegrationTests/custom-setup/custom-setup-without-cabal-defaultMain/Setup.hs
tests/IntegrationTests/custom-setup/custom-setup-without-cabal-defaultMain/custom-setup-without-cabal-defaultMain.cabal
tests/IntegrationTests/custom-setup/custom-setup-without-cabal/Setup.hs
tests/IntegrationTests/custom-setup/custom-setup-without-cabal/custom-setup-without-cabal.cabal
tests/IntegrationTests/custom-setup/custom-setup/Setup.hs
tests/IntegrationTests/custom-setup/custom-setup/custom-setup.cabal
tests/IntegrationTests/custom-setup/custom_setup_without_Cabal_doesnt_allow_Cabal_import.sh
tests/IntegrationTests/custom-setup/custom_setup_without_Cabal_doesnt_require_Cabal.sh
tests/IntegrationTests/custom-setup/installs_Cabal_as_setup_dep.sh
tests/IntegrationTests/custom/custom_dep.sh
tests/IntegrationTests/custom/custom_dep/client/B.hs
tests/IntegrationTests/custom/custom_dep/client/Setup.hs
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
name: Cabal
version: 99998
build-type: Simple
cabal-version: >= 1.2

library
build-depends: base
exposed-modules: CabalMessage
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module CabalMessage where

message = "This is Cabal-99998"
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
name: Cabal
version: 99999
build-type: Simple
cabal-version: >= 1.2

library
build-depends: base
exposed-modules: CabalMessage
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module CabalMessage where

message = "This is Cabal-99999"
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import Distribution.Simple

main = defaultMain
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
name: custom-setup-without-cabal-defaultMain
version: 1.0
build-type: Custom
cabal-version: >= 1.2

custom-setup
setup-depends: base

library
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import System.Exit
import System.IO

main = hPutStrLn stderr "My custom Setup" >> exitFailure
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
name: custom-setup-without-cabal
version: 1.0
build-type: Custom
cabal-version: >= 99999

custom-setup
setup-depends: base

library
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import CabalMessage (message)
import System.Exit
import System.IO

main = hPutStrLn stderr message >> exitFailure
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
name: custom-setup
version: 1.0
build-type: Custom
cabal-version: >= 99999

custom-setup
setup-depends: base, Cabal >= 99999

library
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
. ./common.sh
cd custom-setup-without-cabal-defaultMain

# This package has explicit setup dependencies that do not include Cabal.
# Compilation should fail because Setup.hs imports Distribution.Simple.
! cabal new-build custom-setup-without-cabal-defaultMain > output 2>&1
cat output
grep -q "\(Could not find module\|Failed to load interface for\).*Distribution\\.Simple" output \
|| die "Should not have been able to import Cabal"

grep -q "It is a member of the hidden package .*Cabal-" output \
|| die "Cabal should be available"
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
. ./common.sh
cd custom-setup-without-cabal

# This package has explicit setup dependencies that do not include Cabal.
# new-build should try to build it, even though the cabal-version cannot be
# satisfied by an installed version of Cabal (cabal-version: >= 99999). However,
# configure should fail because Setup.hs just prints an error message and exits.
! cabal new-build custom-setup-without-cabal > output 2>&1
cat output
grep -q "My custom Setup" output \
|| die "Expected output from custom Setup"
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Regression test for issue #3436

. ./common.sh
cabal sandbox init
cabal install ./Cabal-99998
cabal sandbox add-source Cabal-99999

# Install custom-setup, which has a setup dependency on Cabal-99999.
# cabal should build the setup script with Cabal-99999, but then
# configure should fail because Setup just prints an error message
# imported from Cabal and exits.
! cabal install custom-setup/ > output 2>&1

cat output
grep -q "This is Cabal-99999" output || die "Expected output from Cabal-99999"