Assertion failure with --package-db flag

[mpickering]

If you specify the --package-db flag then you get an assertion failure whenever cabal attempts to build a package which should end up in the store.

Repro: https://github.com/mpickering/cautious-octo-spoon

The assertion checks whether the elabRegisterPackageDBStack is the same as storePackageDBStack, which they are not because the elabRegisterPackageDBStack contains the --package-db flag.

The reason for this difference is that in ProjectPlanning.hs computes corePackageDbs as follows:

2318       corePackageDbs =                                                          
2319         applyPackageDbFlags                                                     
2320           (storePackageDBStack compiler)                                        
2321           (projectConfigPackageDBs sharedPackageConfig)  

Notice that the project config package db flags are applied after the store package db paths (this seems wrong to me).

Things only don't go horribly wrong because in the next bit of code storePackageDBStack is used to decide where to register the package rather than elabRegisterPackageDBStack.

In order to understand how to fix this assertion it's necessary to decide on the specification of the --package-db flag.

My understanding of how I want it to work is that --package-db flag specifies an immutable package database which augments the global store with some additional packages. We do not under any circumstances want to register packages into a package database provided by --package-db.

cabal-install mutates two package databases:

• The store directory
• The inplace directory

Therefore these should always appear after the package database provided by --package-db so that when a package is registered it's registered into one of these two places rather than anything the user provides with --package-db.

This also has the consequence that packages in the store can depend on --package-db packages, but not vice-versa, which I believe is also the intention of the flag.

Flag introduced in : #7676

In the documentation it states that the order is intentional but I don't think that makes much sense:

    This flag manipulates the default prefix: ``[global, store]`` and accepts
    paths, the special value ``global`` referring to the global package db, and
    ``clear`` which removes all prior entries. For example,

    ::

        -- [global, store, foo]
        package-dbs: foo

[mpickering]

I have a patch which fixes things in the way described.

I think it's clearer if the semantics of the flag don't allow you to remove the store directory from the search path (there's no way to add it back in later..). The notion of the cabal store is quite baked into cabal and can be modified using the --store-dir flag. Therefore there doesn't seem much value is adding a second way to override it by specifying package-dbs.

[mpickering]

It seems there is also a potential bug with things in the CmdInstall module not respecting the --package-db flag. This I identified by applying the flag to the storePackageDBStack in StoreDirLayout rather than at the point of use and observing that this module initialised this data type.

[fgaz]

Your analysis seems correct to me, and I agree with the proposed fix 👍

[mpickering]

Duncan instead suggests that it would be better to extend the syntax of --package-dbs to allow you to be explicit about where store is located in the package db stack.

To me this seems like a worse way to go because the notion of a store is quite baked into cabal and it seems that allowing the user the control over which package database packages get installed into is too low-level for cabal-install.

[andreabedini]

This also has the consequence that packages in the store can depend on --package-db packages, but not vice-versa, which I believe is also the intention of the flag.

A package in the store depending on an effimeral package-db provided on the command line is basically broken, since there is no guarantee that the package-db will stay around.

Note that the store does not work like a regular package-db. It's invibile to the solver and it is only used to replace configured packages with pre-built ones. This is done just by looking at the package hash so there is no consistency check in the store.

[mpickering]

I think it is up to the user to guarantee that package databases passed with --package-db stay around. The same is true for the global package db, which may disappear if you uninstall your compiler but keep the store.

The package hash contains the package db stack which was in place when the package was built, which I think guards against using packages built with --package-db flags in play if you later remove the options. Is that the situation you are worried about?

What is your conceptual model of how the --package-db flag should work @andreabedini, I'm interested to hear what you think so that we can nail down a specification which can be made robust into the future.

[andreabedini]

The package hash contains the package db stack which was in place when the package was built, which I think guards against using packages built with --package-db flags in play if you later remove the options. Is that the situation you are worried about?

That's right, I worry that it could be fragile though; as soon as we see the same package-db, we will happy swap the configured package with the pre-installed one in the store without any extra checks.

What if the package-db is still there but one package has been removed? or (perhaps worse) changed/rebuilt?

What if the user wipes the package-db and rebuilds it (with whatever tool their are using to do so)? Packages built with that package-db might not work anymore.

With the cabal store we can make assumptions on the meaning of the unit id (containing a digest of the package inputs) but with external package-dbs we cannot do that.

I have plenty of experience with users build packages inside a nix shell; which I think it's similar. Packages in the cabal store will end up referencing dynamic libraries in the nix store; this was invibile to cabal so the only was to remove a package from the store, and then also delete everything that depended on it; at which point rm -rf $CABAL_DIR/store becomes quicker.

What is your conceptual model of how the --package-db flag should work @andreabedini, I'm interested to hear what you think so that we can nail down a specification which can be made robust into the future.

I think I agree with your analysis. I haven't thought much about it but I think it is a mistake that package-db can explicitly mention the store. My only disagreement is about the order, IMHO extras package-db should be between the store db and the in-place db.

Note that we customise how we install packages in the store already, so it would not be a problem adjusting/extending that custom logic.

[mpickering]

@andreabedini I think all your arguments apply to the global package database as well, there's no reason that couldn't be mutated, rebuilt, packages added, removed etc. (And that's what seems to happen in the nix case)

Say that we put the --package-db flags after the store directory, so that things in the store can't depend on these packages. Then when are these packages in this package database going to be used?

• Will the solver still use package versions given in your new package database? Well, it can't use them to satisfy the dependency of anything which is going to be put into the store.
• Where does cabal install packages? Into your given package database or into the store? Related to the question above, if you are allowed to use your new packages in an install plan then you have to register the packages into your new package database (or one which comes after your new package database).

So it seems to me the --package-db flag is a bit useless in that case.

OTOH, you are explaining that the flag is rightfully dangerous, you will end up with broken packages in your store if you modify or mutate the package database. Therefore I propose the following semantics for the flag.

• --package-db flag specifies the path to a read-only, immutable package database which is inserted into the package database path after the global package database (but before the store). You should think of any --package-db you specify as an extension to the global package database.

The --package-db path is immutable, and contributes to the store hash. These packages will only be used in situations where the user has explicitly passed --package-db, so they exist separately from "normal" invocations which just rely on the global package database. If you delete the package database, then obviously invoking cabal with --package-db will not work. You are not permitted to mutate the package database (as is already the implicit contract with the global package database).

Thoughts?

[andreabedini]

I need to think this through, and perhaps double check the code. For the moment I will leave a couple of comments to be sure we are on the same page.

We use PackageDBStack in different ways in the codebase and I have doubts there is a single unifying picture that encompass both V1 and V2 commands. As you must have noticed, in ElaboratedConfiguredPackage we have

  , elabPackageDbs :: [Maybe PackageDB]
  , elabSetupPackageDBStack :: PackageDBStack
  , elabBuildPackageDBStack :: PackageDBStack
  , elabRegisterPackageDBStack :: PackageDBStack
  , elabInplaceSetupPackageDBStack :: PackageDBStack
  , elabInplaceBuildPackageDBStack :: PackageDBStack
  , elabInplaceRegisterPackageDBStack :: PackageDBStack

which shows we use different a PackageDBStack dependending on the situation.

I am pretty sure the solver has zero knowledge of the store, as I said before, the store should not be considered a packagedb. I need to check but I think getInstalledPackages is never called on the store directory. So the solver will use any package from the packagedb at its disposal which by default (in the V2 code path) is only the global package-db.

-- [global, store, foo]
package-dbs: foo

Note that store is not a special value for packagedb, it's parsed as a path.

Where does cabal install packages? Into your given package database or into the store?

Well the short answer these days is: cabal does not install anything 😬 v2 commands do not really install packages as v1 commands did.
It is true that cabal install --lib creates a environment file that references the store packagedb, but it is not a packagedb you are supposed to manage yourself.

Related to the question above, if you are allowed to use your new packages in an install plan then you have to register the packages into your new package database (or one which comes after your new package database).

What is going to happen is that a pre-installed package fixes all its dependencies. There are two cases:

1. the packagedb is self-contained, in this case the presence of the store is irrelevant since all dependencies of anything coming from a packagedb will be in that same packagedb (and won't ever be replaced with anything from the store).
2. a package in the packagedb has dangling references, e.g. to the store. In this case the solver has to know about the store otherwise it will dismiss those packages as broken.

We need to understand the scenario we want to support. I am afraid that we do not support case 2 at the moment.

PS: Perhaps my previous comments about the order were premature. Order in what list? The store is not passed as a packagedb to the solver, but it is passed as a packagedb to the compiler (along with -hide-all-packages and an explicit list of package-ids so that GHC does no guessing).

PPS: I haven't looked at the assertion yet.

[mpickering]

To clarify @andreabedini

I think we have a bit of a misunderstanding about my two questions:

1. By "install" I don't mean "v2-install" or "v1-install" but build and register a package when building a dependency (into the store). I think you are probably assuming that cabal will ALWAYS build packages into the store (which is not what the current documentation for -package-db says. I think we are in agreement that cabal should always write it's packages into the store and -package-db flag should not affect this.
2. I was more concerned about whether the build plan will be allowed to depend on packages in the extra specified package db.
   • --package-db flags are after store: This is impossible because the order [global, store, pkg-db] means that things in the store can't depend on pkg-db. So you will only be able to use the extra packages for inplace builds which severally limits the utility. What is the use-case for -package-db flag in this case?
   • -package-db flags are before the store: Build plan can depend on your extra packages to build dependencies, as the order is correct.

I think that getInstalledPackages must be called at some point on the store, as otherwise you just don't know what's in there? In any case, I do realise that store packages are treated differently from global package db packages (as you describe).

[andreabedini]

⚠️ I am not sure what I say here is all correct, this is mostly the result of me trying to understand how things work (or are supposed to work).

I think we have a bit of a misunderstanding about my two questions:

I realise now I have been following your chain of thoughts backward (the pr, the issue, the asertion). Thank for raising this up.

1. By "install" I don't mean "v2-install" or "v1-install" but build and register a package when building a dependency (into the store). I think you are probably assuming that cabal will ALWAYS build packages into the store (which is not what the current documentation for -package-db says. I think we are in agreement that cabal should always write it's packages into the store and -package-db flag should not affect this.

Yes, we do agree. If elabRegisterPackageDBStack pkg != storePackageDBStack compiler it means something is gone wrong adding packagedbs to the different stacks. You also correct in that I learned how this works by reading the code and not by reading the documentation 😂

Let me swap the next two paragraphs around.

I think that getInstalledPackages must be called at some point on the store, as otherwise you just don't know what's in there? In any case, I do realise that store packages are treated differently from global package db packages (as you describe).

We really don't call getInstalledPackages on the storePackageDBStack ... other than in CmdInstall 😂 let me get back to that.

In a typical cabal build setting, we do not call getInstalledPackages but getStoreEntries which is basically getDirectoryContents. It works like this: project configuration -> solver plan (by package) -> elaborated plan (by components); so far the store does not exist. Then from the elaborated plan we compute the package hashes, and any unit in elaborated plan whose package hash matches one from the store gets marked as pre-installed rather than configured.

-- | Given the 'InstalledPackageIndex' for a nix-style package store, and an
-- 'ElaboratedInstallPlan', replace configured source packages by installed
-- packages from the store whenever they exist.
improveInstallPlanWithInstalledPackages
  :: Set UnitId
  -> ElaboratedInstallPlan
  -> ElaboratedInstallPlan
improveInstallPlanWithInstalledPackages installedPkgIdSet =
  InstallPlan.installed canPackageBeImproved
  where
    canPackageBeImproved pkg =
      installedUnitId pkg `Set.member` installedPkgIdSet

Note that plan.json does not show this as it is printed from the elaborated plan, rather than the "improved" plan.

Now, I did check and in CmdInstall we do something a bit different! TBH I don't know why, it looks like we are reading packagedbs from the ghc environment file? This looks like a very impure way of building packages.

I was more concerned about whether the build plan will be allowed to depend on packages in the extra specified package db.

--package-db flags are after store: This is impossible because the order [global, store, pkg-db] means that things in the store can't depend on pkg-db. So you will only be able to use the extra packages for inplace builds which severally limits the utility. What is the use-case for -package-db flag in this case?
-package-db flags are before the store: Build plan can depend on your extra packages to build dependencies, as the order is correct.

Ok, I had to refresh on GHC's behaviour and I had neglected the hierarchy of packagedbs.

This is what I think is the design (and the implementation, a part from bugs):

• the build plan does not know about the store (sorry if I keep repeating this)
• the store is appended last when we call ghc --make
• GHC will reject extra packagedbs which try to depend on the store (because the are lower in the stack) but the solver will have discarded them already.
• in-place packagedbs are added after the store, the can depend on the store because they either are going to be part of the store themselves or nobody will depend on them.

Therefore: [global packagedb, project packagedbs, store packagedb, in-place packagedb (when relevant)]

This confirms that we cannot support the scenario 2 in my previous comment; i.e. you cannot cabal v2-install something in a packagedb that you can reuse later.

... and that, as you started with,

      corePackageDbs =
        applyPackageDbFlags
          (storePackageDBStack compiler)
          (projectConfigPackageDBs sharedPackageConfig)

must be wrong.

[mpickering]

Thank you for pointing that out about the store, so that confirms that indeed it is very special and shouldn't be overridden in this ad-hoc way by the user.

So we are agreed on principle 1, --package-db does not affect store.

About principle 2, the order the --package-db flags apply, I think we also agree?

• --package-db flags CAN'T be after the store, because then nothing can depend on packages in these package databases (which seems to defeat the purpose of this flag)
• Therefore --package-db flags get put before the store, and are only used to influence the initial set of global packages which the solver can choose from. With the assumption that these package database are immutable and will not be modified.

And in that case, my patch in #9683 implements this proposal.

[andreabedini]

Yes, I agree with all that. Thank you for taking on this.