Skip to content

Commit

Permalink
Actual implementation of '--exact-configuration'.
Browse files Browse the repository at this point in the history
  • Loading branch information
@23Skidoo
23Skidoo committed Dec 5, 2013
1 parent 5583563 commit d87b584
Showing 1 changed file with 33 additions and 19 deletions.
52 changes: 33 additions & 19 deletions Cabal/Distribution/Simple/Configure.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,9 +339,30 @@ configure (pkg_descr0, pbi) cfg
installedPackageSet <- getInstalledPackages (lessVerbose verbosity) comp
packageDbs programsConfig'

let -- Constraint test function for the solver
dependencySatisfiable =
not . null . PackageIndex.lookupDependency pkgs'
(allConstraints, requiredDepsMap) <- either die return $
combinedConstraints (configConstraints cfg)
(configDependencies cfg)
installedPackageSet

let exactConf = fromFlagOrDefault False (configExactConfiguration cfg)
-- Constraint test function for the solver
dependencySatisfiable d@(Dependency depName verRange)
| exactConf =
-- When we're given '--exact-configuration', we assume that all
-- dependencies and flags are exactly specified on the command
-- line. Thus we only consult the 'requiredDepsMap'. Note that
-- we're not doing the version check.
--
-- TODO: mention '--exact-configuration' in the error message
-- when this fails?
(depName `Map.member` requiredDepsMap)
|| (pkgName pid == depName
&& pkgVersion pid `withinRange` verRange)

This comment has been minimized.

Sign in to view

Copy link
@dcoutts

dcoutts Dec 6, 2013

Ahh, this is an internal dependency. Could probably give that a named local function so it's clearer.

This comment has been minimized.

Sign in to view

Copy link
@23Skidoo

23Skidoo Dec 6, 2013

Author Owner

Fixed.

| otherwise =
-- Normal operation: just look up dependency in the package
-- index.
not . null . PackageIndex.lookupDependency pkgs' $ d
where
pkgs' = PackageIndex.insert internalPackage installedPackageSet
enableTest t = t { testEnabled = fromFlag (configTests cfg) }
Expand All @@ -355,11 +376,6 @@ configure (pkg_descr0, pbi) cfg
pkg_descr0'' = pkg_descr0 { condTestSuites = flaggedTests
, condBenchmarks = flaggedBenchmarks }

(allConstraints, requiredDepsMap) <- either die return $
combinedConstraints (configConstraints cfg)
(configDependencies cfg)
installedPackageSet

(pkg_descr0', flags) <-
case finalizePackageDescription
(configConfigurationsFlags cfg)
Expand Down Expand Up @@ -655,17 +671,15 @@ selectDependency internalIndex installedIndex requiredDepsMap
[(_,[pkg])] | packageVersion pkg `withinRange` vr
-> Right $ InternalDependency dep (packageId pkg)

_ -> case PackageIndex.lookupDependency installedIndex dep of
[] -> Left $ DependencyNotExists pkgname
pkgs -> Right $ ExternalDependency dep $
case Map.lookup pkgname requiredDepsMap of
-- if we know the exact pkg to use then use it
Just pkginstance -> pkginstance
-- otherwise we just pick an arbirary instance of the
-- latest version
Nothing ->
case last pkgs of
(_ver, pkginstances) -> head pkginstances
_ -> case Map.lookup pkgname requiredDepsMap of
-- If we know the exact pkg to use, then use it.
Just pkginstance -> Right (ExternalDependency dep pkginstance)
-- Otherwise we just pick an arbitrary instance of the latest version.
Nothing -> case PackageIndex.lookupDependency installedIndex dep of
[] -> Left $ DependencyNotExists pkgname
pkgs -> Right $ ExternalDependency dep $
case last pkgs of
(_ver, pkginstances) -> head pkginstances

reportSelectedDependencies :: Verbosity
-> [ResolvedDependency] -> IO ()
Expand Down

3 comments on commit d87b584

@dcoutts
Copy link

@dcoutts dcoutts commented on d87b584 Dec 6, 2013

Choose a reason for hiding this comment

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

Hmm, a few issues here.

Firstly, we're not actually relaxing the constraints from the .cabal file, so the solver (in finalizePackageDescription) should fail if we give a dep on a package that's not allowed by the constraints in the .cabal file. OTOH, you say it works, but my reading is that it ought to fail, so what am I missing?

Secondly, I think for --exact-configuration we ought to have a sanity check that all the deps and flags are indeed specified.

I realise that finalizePackageDescription is rather limiting, but eventually (if I ever get any further with my parser + GenericPackageDescription rewrite) we'll have finalizePackageDescription split into two: a solver and a function that applies a solution to go from a GenericPackageDescription to a PackageDescription. With --exact-configuration we will be able to skip the solver bit and go straight to a solution, but obviously that relies on the solution being complete. So I think the sanity check is important (cabal-install could get it wrong and specify incomplete info which would be partially covered up by the finalizePackageDescription filling in the remaining info -- leading to really hard to diagnose bug reports).

@23Skidoo
Copy link
Owner Author

Choose a reason for hiding this comment

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

I changed how dependencySatisfiable works. With --exact-configuration, it simply looks up the package name in the requiredDepsMap. Note that it does not do the version range check. This has the added benefit that if the package name is not in requiredDepsMap (wasn't given via --dependency), finalizePackageDescription will fail because the dependency is not satisfiable.

So I think that in the sanity check I need to only look at whether all flags are provided.

@dcoutts
Copy link

@dcoutts dcoutts commented on d87b584 Dec 9, 2013

Choose a reason for hiding this comment

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

Ok, got it, thanks.

Please sign in to comment.