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

Fix non-buildable bug in Cabal <= 1.22 #3632

Merged
merged 1 commit into from
Dec 10, 2017
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
16 changes: 16 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,21 @@
# Changelog

## Unreleased changes

Release notes:

Major changes:

Behaviour changes:

Other enhancements:

Bug fixes:

* For versions of Cabal before 1.24, ensure that the dependencies of
non-buildable components are part of the build plan to work around an old
Cabal bug.

## v1.6.1

Major changes:
Expand Down
2 changes: 1 addition & 1 deletion src/Stack/BuildPlan.hs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ gpdPackageDeps
-> Map FlagName Bool
-> Map PackageName VersionRange
gpdPackageDeps gpd cv platform flags =
Map.filterWithKey (const . (/= name)) (packageDependencies pkgDesc)
Map.filterWithKey (const . (/= name)) (packageDependencies cv pkgDesc)
where
name = gpdPackageName gpd
-- Since tests and benchmarks are both enabled, doesn't matter
Expand Down
30 changes: 27 additions & 3 deletions src/Stack/Package.hs
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ packageFromPackageDescription packageConfig pkgFlags (PackageDescriptionPair pkg
pkgId = package pkg
name = fromCabalPackageName (pkgName pkgId)
deps = M.filterWithKey (const . not . isMe) (M.union
(packageDependencies pkg)
(packageDependencies (packageConfigCompilerVersion packageConfig) pkg)
-- We include all custom-setup deps - if present - in the
-- package deps themselves. Stack always works with the
-- invariant that there will be a single installed package
Expand Down Expand Up @@ -602,12 +602,36 @@ getBuildComponentDir Nothing = Nothing
getBuildComponentDir (Just name) = parseRelDir (name FilePath.</> (name ++ "-tmp"))

-- | Get all dependencies of the package (buildable targets only).
packageDependencies :: PackageDescription -> Map PackageName VersionRange
packageDependencies pkg =
--
-- Note that for Cabal versions 1.22 and earlier, there is a bug where
-- Cabal requires dependencies for non-buildable components to be
-- present. We're going to use GHC version as a proxy for Cabal
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't see the reasoning of using a GHC version as a proxy for the cabal version. AFAIU you can use Cabal 2 with GHC 7.8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered going that route, but it's a much bigger diff, and gets into a bit of a circular issue, since we need to know the version of the Cabal library before we've finished calculating the build plan. The way I've written it, we'll occasionally (but rarely) get some unnecessary extra packages built if someone is using old GHC + new Cabal, but won't get any breakage (since you can't use old Cabal with a new GHC).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see! Alright fair 'nough

-- library version in this case for simplicity, so we'll check for GHC
-- being 7.10 or earlier. This obviously makes our function a lot more
-- fun to write...
packageDependencies
:: CompilerVersion 'CVActual
-> PackageDescription
-> Map PackageName VersionRange
packageDependencies ghcVersion pkg' =
M.fromListWith intersectVersionRanges $
map (depName &&& depRange) $
concatMap targetBuildDepends (allBuildInfo' pkg) ++
maybe [] setupDepends (setupBuildInfo pkg)
where
pkg
| getGhcVersion ghcVersion >= $(mkVersion "8.0") = pkg'
-- Set all components to buildable. Only need to worry about
-- library, exe, test, and bench, since others didn't exist in
-- older Cabal versions
| otherwise = pkg'
{ library = (\c -> c { libBuildInfo = go (libBuildInfo c) }) <$> library pkg'
, executables = (\c -> c { buildInfo = go (buildInfo c) }) <$> executables pkg'
, testSuites = (\c -> c { testBuildInfo = go (testBuildInfo c) }) <$> testSuites pkg'
, benchmarks = (\c -> c { benchmarkBuildInfo = go (benchmarkBuildInfo c) }) <$> benchmarks pkg'
}

go bi = bi { buildable = True }

-- | Get all dependencies of the package (buildable targets only).
--
Expand Down
2 changes: 1 addition & 1 deletion src/Stack/Snapshot.hs
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ calculate gpd platform compilerVersion loc flags hide options =
, lpiGhcOptions = options
, lpiPackageDeps = Map.map fromVersionRange
$ Map.filterWithKey (const . (/= name))
$ packageDependencies pd
$ packageDependencies compilerVersion pd
, lpiProvidedExes =
Set.fromList
$ map (ExeName . T.pack . C.unUnqualComponentName . C.exeName)
Expand Down
18 changes: 18 additions & 0 deletions test/integration/tests/cabal-non-buildable-bug/Main.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import StackTest

main :: IO ()
main = do
-- Newer Cabal: dry run and building should succeed, because they'll
-- both ignore the do-not-build
writeFile "stack.yaml" "resolver: ghc-8.0.2"
stack ["build", "--dry-run"]
stack ["build"]

-- Older Cabal: both should fail, because they'll both try to
-- include the non-buildable component. If there's a regression, the
-- dry run will succeed (because Stack will use the proper logic)
-- and build will fail (because Cabal will be using its broken
-- logic).
writeFile "stack.yaml" "resolver: ghc-7.10.3"
stackErr ["build"]
stackErr ["build", "--dry-run"]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
stack.yaml
foo.cabal
14 changes: 14 additions & 0 deletions test/integration/tests/cabal-non-buildable-bug/files/package.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
name: foo
version: "0"

dependencies:
- base

library: {}

executables:
not-built:
main: Main.hs
dependencies:
- does-not-exist
buildable: false