Closed snowleopard closed 5 years ago
I see a lot of cosmetic changes but between that large-but-mostly-irrelevant diff and the very short comment, I have a hard time figuring out what is it that you actually change to correctly figure out package dependencies, do you mind giving me a very quick summary?
@alpmestan My apologies about making this PR difficult to review. To figure out the failure in #654 I had to read some code I didn't write and my hands were automatically fixing some style inconsistencies with the rest of the Hadrian code base. (I'd like to emphasise that I make no judgement about which style is better, it's merely an attempt to make the code base look more homogeneous, which I think is useful.)
There are only two important changes.
1) The first one fixes missing dependencies:
-pkgDependencies = fmap (fmap PD.dependencies) . readPackageDataFile
+pkgDependencies = fmap (fmap (map pkgName . packageDependencies)) . readCabalFile
Here PD.dependencies
returned versioned package names, e.g. ghc-8.7
, which then failed to match with non-versioned package names such as ghc
in contextDependencies
. Switching from PD.dependencies
to packageDependencies
fixes this.
2) I clearly remember that we didn't have this bug before, so I added some tests for our package dependency infrastructure to prevent such regressions in future:
testDependencies :: Action ()
testDependencies = do
putBuild "==== pkgDependencies"
depLists <- mapM (pkgDependencies . vanillaContext Stage1) ghcPackages
test $ and [ deps == sort deps | Just deps <- depLists ]
putBuild "==== Dependencies of the 'ghc-bin' binary"
ghcDeps <- pkgDependencies (vanillaContext Stage1 ghc)
test $ isJust ghcDeps
test $ pkgName compiler `elem` fromJust ghcDeps
stage0Deps <- contextDependencies (vanillaContext Stage0 ghc)
stage1Deps <- contextDependencies (vanillaContext Stage1 ghc)
stage2Deps <- contextDependencies (vanillaContext Stage2 ghc)
test $ vanillaContext Stage0 compiler `notElem` stage1Deps
test $ vanillaContext Stage1 compiler `elem` stage1Deps
test $ vanillaContext Stage2 compiler `notElem` stage1Deps
test $ stage1Deps /= stage0Deps
test $ stage1Deps == stage2Deps
Everything else are cosmetic changes, fixing minor issues in comments, and adding TODOs.
Hope this helps!
@alpmestan Many thanks for the review! Merged.
See #654