cabal-install: Clarify the semantics of package-db flag #9683
Conversation
| library | ||
| import: warnings | ||
| exposed-modules: MyLib | ||
| build-depends: base ^>=4.18.0.0 |
There was a problem hiding this comment.
Doesn't this restrict the ghc versions this test can be run with?
mpickering
force-pushed
the
wip/package-db-fix
branch
2 times, most recently
from
bcdee26 to
f1e14f7
Compare
| cabalDirLayout <- mkCabalDirLayout mstoreDir mlogsDir | ||
| cabalDirLayout <- mkCabalDirLayout mstoreDir mlogsDir projectConfigPackageDBs |
There was a problem hiding this comment.
This should raise eyebrows. How come the CABAL_DIR layout now depends on project level configuration?
There was a problem hiding this comment.
Because the initial package database stack depends on the package-db option, which is specified at a project level.
This seems completely natural to me, and changing the implementation like this caught the fact that --package-db flag was not honoured by the install command. What is your proposed alternative?
There was a problem hiding this comment.
The layout of CABAL_DIR does not depend on any project configuration. What happens here is that CabalDirLayout contains (through StoreDirLayout)
storePackageDBStack :: Compiler -> PackageDBStack
storePackageDBStack compiler = [GlobalPackageDB, storePackageDB compiler]
You want to insert project packagedbs inside that stack and you cannot do it. IMHO the right solution is either
- Change that function to take a list of packagedb (just like it takes a compiler):
storePackageDBStack :: Compiler -> [PackageDB] -> PackageDBStack
storePackageDBStack compiler packagedbs = GlobalPackageDB : packagedbs ++ [storePackageDB compiler]
(modulo a Just, I think we should only allow "real" packagedbs rather than "maybe encoded packagedb commands")
or
- Remove that function entirely and rely on
storePackageDB :: Compiler -> PackageDBinstead. The accessorstorePackageDBStackdoes not really save any code and you just have proven that it was hiding an important decision (the order).
There was a problem hiding this comment.
That is a good idea to change the function to take an extra argument. I have done that.
mpickering
force-pushed
the
wip/package-db-fix
branch
from
f1e14f7 to
66b1460
Compare
|
@andreabedini and I agree on the specification, I have modified the patch based on Andrea's review, can we get a second approval please so I can merge this patch? |
| applyPackageDbFlags | ||
| [GlobalPackageDB] | ||
| (projectConfigPackageDBs projectConfigShared) | ||
| Cabal.interpretPackageDbFlags False (projectConfigPackageDBs projectConfigShared) |
There was a problem hiding this comment.
Why is GlobalPackageDB not needed here? Is it already present in projectConfigPackageDBs?
There was a problem hiding this comment.
It is added by interpretPackageDbFlags.
|
Thanks @andreabedini |
mergify
bot
added
the
merge delay passed
In this PR we make the `--package-db` flag apply only to the default immutable initial package stack rather than also applying to the store package database. There are two special package databases which cabal install knows about and treats specially. * The store directory, a global cache of installed packages where cabal builds and installs packages into. * The inplace directory, a local build folder for packages, where cabal builds local packages in. It is very important that cabal registers packages it builds into one of these two locations as there are many assumptions that packages will end up here. With the current design of the `--package-db` flag, you are apparently allowed to override the store location which should have the effect of making the last package database in the package stack the "store" directory. Perhaps this works out in theory but practically this behaviour was broken and things were always registered into the store directory that cabal knew about. (The assertion in `ProjectBuilding.UnpackedPackage` was failing (see added test)). With `--package-db` not being able to modify the location of the store directory then the interaction of `--package-db`, `--store-dir` and `--dist-dir` flags become easy to understand. * `--package-db` modify the initial package database stack, these package database will not be mutated by cabal and provide the initial package environment which is used by cabal. * `--store-dir` modify the location of the store directory * `--dist-dir` modify the location of the dist directory (and hence inplace package database) Treating the flags in this way also fix an assertion failure when building packages. Fixes haskell#9678
Mikolaj
force-pushed
the
wip/package-db-fix
branch
from
66b1460 to
d626ef8
Compare
In this PR we make the
--package-dbflag apply only to the default immutable initial package stack rather than also applying to the store package database.There are two special package databases which cabal install knows about and treats specially.
It is very important that cabal registers packages it builds into one of these two locations as there are many assumptions that packages will end up here.
With the current design of the
--package-dbflag, you are apparently allowed to override the store location which should have the effect of making the last package database in the package stack the "store" directory. Perhaps this works out in theory but practically this behaviour was broken and things were always registered into the store directory that cabal knew about. (The assertion inProjectBuilding.UnpackedPackagewas failing (see added test)).With
--package-dbnot being able to modify the location of the store directory then the interaction of--package-db,--store-dirand--dist-dirflags become easy to understand.--package-dbmodify the initial package database stack, these package database will not be mutated by cabal and provide the initial package environment which is used by cabal.--store-dirmodify the location of the store directory--dist-dirmodify the location of the dist directory (and hence inplace package database)Treating the flags in this way also fix an assertion failure when building packages.
Please read Github PR Conventions and then fill in one of these two templates.
Template Α: This PR modifies
cabalbehaviourInclude the following checklist in your PR:
Template Β: This PR does not modify
cabalbehaviour (documentation, tests, refactoring, etc.)Include the following checklist in your PR: