Skip to content
This repository

Make add-source packages have priority over other packages #1197

Closed
mightybyte opened this Issue February 12, 2013 · 24 comments

4 participants

Doug Beardsley Mikhail Glushenkov Johan Tibell Duncan Coutts
Doug Beardsley

I described the following scenario in issue #1195.

I installed my package foo and it retrieved a dependency bar from hackage. Then I discovered that I in fact needed my locally modified version of bar. So I do cabal sandbox-add-source ../bar. But that doesn't do anything since it already has the version from hackage which satisfies the dependencies.

It seems like the version added with add-source should take priority here.

Mikhail Glushenkov
Owner

Thanks for the feedback, I'll look into this.

Doug Beardsley

Actually, this problem exists even when hackage isn't involved. If I do cabal sandbox-add-source ../bar and then build foo, bar gets installed into the sandbox. Then if I make a change to bar, that change is not visible when I rebuild foo, so I'm forced to unregister bar from foo's build sandbox. I also tried running sandbox-add-source for bar again, but that didn't work either.

Mikhail Glushenkov
Owner

Are you using sandbox-build to build your project?

Doug Beardsley

I'm using sandbox-install. I figured that would be more appropriate given what I already know about the non-sandbox install/build distinction.

Mikhail Glushenkov
Owner

Can you provide me with step-by-step instructions for how to reproduce your problem?

Doug Beardsley
  1. Create a project bar that exports some symbol barSymbol.
  2. Create a project foo that depends on bar and uses barSymbol.
  3. Run cabal sandbox-init in foo's project directory.
  4. Use cabal sandbox-add-source to add the bar project to the foo project's sandbox.
  5. Build foo with cabal sandbox-install
  6. Modify bar to make some change to the exported barSymbol.
  7. Rebuild bar to make sure it compiles (note, that you can skip this step without changing the behavior).
  8. Rebuild foo with cabal sandbox-install. It will not see the changes made to barSymbol.
  9. Try doing cabal sandbox-add-source ../bar again to see if that might cause it to be rebuilt.
  10. Rebuild foo again with cabal sandbox-install and find there are still no changes.
Mikhail Glushenkov
Owner

@mightybyte Thanks!

Mikhail Glushenkov
Owner

A simple solution is to just unregister the already-existing version. See also #1275.

Doug Beardsley

I completely agree, but there is no facility for unregistering packages in a sandbox.

Mikhail Glushenkov
Owner

@mightybyte I was talking about internal implementation. To do this manually you can use the sandbox hc-pkg command that I implemented recently.

Doug Beardsley

Oh, excellent. I hadn't seen that.

Mikhail Glushenkov
Owner

I'm not sure if we can solve this completely automatically. add-source is not supposed to install packages into the sandbox, but that would be required to make this work without manual intervention. This is complicated by the fact that the sandbox can contain package DBs for several different compilers, and we would need to update all of them.

I suggest that we make add-source print a warning when the package is already installed:

$ cabal sandbox add-source /path/to/some-package-0.1
Warning: some-package-0.1 is already installed into the sandbox.
Please unregister some-package-0.1 manually with 'cabal sandbox hc-pkg unregister some-package-0.1'
and then run 'cabal install --reinstall --whole-environment some-package-0.1'.
Johan Tibell
Owner
tibbe commented May 13, 2013

Can we forcefully reinstall add-source dependencies if they have changed, overwriting whatever version was in the sandbox before?

Mikhail Glushenkov
Owner

Can we forcefully reinstall add-source dependencies if they have changed, overwriting whatever version was in the sandbox before?

In add-source, you mean?

What we can do, perhaps, is make add-source set the timestamp of those dependencies that were already installed to some date in the past newer than the sandbox timestamp. In this way, they and their reverse dependencies will be reinstalled automatically on the next cabal build.

Johan Tibell
Owner
tibbe commented May 13, 2013

I think fixing this issue is important. We want sandboxes to behave as-if you deleted the whole package DB and started over from scratch. Now, we don't want to actually do that, as it would be way to slow, so we'll have to come up with a solution. Possible that will include making the dep solver smarter i.e. but letting it know that certain versions have priority over others or that a certain constraint can only be fulfilled by an in-place version of a package.

@kosmikus any ideas?

Mikhail Glushenkov
Owner

I can implement my timestamp idea so that we can experiment with it. Basically, this will be equivalent to the following:

$ cabal sandbox add-source /path/to/some-package-0.1
# Since some-package-0.1 is already installed in the sandbox:
$ touch /path/to/some-package-0.1/some-package.cabal
$ cabal build
Installing add-source dependencies...
Resolving dependencies...
In order, the following will be installed:
some-package-0.1 (reinstall)
Mikhail Glushenkov
Owner

Now, this won't work for snapshots, but I think cabal-dev has the same problem.

Johan Tibell
Owner
tibbe commented May 13, 2013

Lets not discard the "proper" solution (i.e. add a "unpacked"/"local" constraint to the solver, just like we have an "installed" constraint today) too quickly. Lets see how difficult it would be to implement first.

Mikhail Glushenkov
Owner

We can start with a simple solution and then maybe implement a proper one.

Mikhail Glushenkov 23Skidoo referenced this issue from a commit in 23Skidoo/cabal May 13, 2013
Mikhail Glushenkov Make newly added add-source deps override previously installed versions.
WIP.

Implemented by touching the .cabal file of the newly-added add-source dep in
'doAddSource'. This triggers it to be reinstalled on the next 'cabal build'.

Fixes #1197.
9c7b539
Mikhail Glushenkov
Owner

Lets not discard the "proper" solution (i.e. add a "unpacked"/"local" constraint to the solver, just like we have an "installed" constraint today) too quickly.

One problem with the "proper" solution is that the timestamp check will prevent build from entering reinstallAddSourceDeps, thus there will be no chance to run the solver. And in general it's a bit unclear what does it mean to prefer "local" packages, since the ones we have installed in the package DB may very well have been also installed from a "local" source.

Doing reinstall in add-source is problematic in the presence of -w since there is no way to enumerate all package DBs and corresponding compilers.

I think that touching the .cabal file in add-source is the easiest soluton (if slightly hacky).

Johan Tibell
Owner
tibbe commented May 13, 2013

OK. Lets try to get something working in the mean time. We need to ship this thing!

Johan Tibell
Owner
tibbe commented May 14, 2013

@dcoutts tells me that we can already express a preference for local packages. That's what e.g. cabal install ./foo does.

Duncan Coutts
Owner
dcoutts commented May 14, 2013

I'm pretty sure we already have all the control we need in terms of constraints etc.

Look at how we translate the PackageSpecifier into constraints for the solver. For a NamedPackage kind (e.g. cabal install foo) we allow any of installed or available src. For a SpecificSourcePackage kind (e.g. cabal install ./foo/foo.cabal) we add constraints so that it can only pick that specific src package, and not installed instances and not other available src packages of that name.

See standardInstallPolicy from Distribution/Client/Dependency.hs where we do:

  . addConstraints
      (concatMap pkgSpecifierConstraints pkgSpecifiers)

  . addTargets
      (map pkgSpecifierTarget pkgSpecifiers)

  . hideInstalledPackagesSpecificBySourcePackageId
      [ packageId pkg | SpecificSourcePackage pkg <- pkgSpecifiers ]

  . addSourcePackages
      [ pkg  | SpecificSourcePackage pkg <- pkgSpecifiers ]

As you can see, for the SpecificSourcePackage kind we add them to the solver's source package set.
Doing that will replace (effectively masking) other source packages of the same name & version. We additionally constraint it to pick that specific source version, so that masks out other packages of the same name but different versions (that's what pkgSpecifierConstraints is doing).

You can also see that for SpecificSourcePackages we hide installed packages that have the same source package id. That's how we make sure that when you do: cabal install ./foo/foo.cabal that it'll re-install that src version and not pick up any installed instance. Obviously for the sandbox case we don't want that latter part, it would be ok to re-use the installed instance if it's consistent with everything else, so it's ok to let the solver consider that.

Mikhail Glushenkov
Owner

Yes, when we know that some add-source deps should be reinstalled then reinstalling them works absolutely fine. The problem is that the timestamp check prevents us from attempting to reinstall them.

I'm currently thinking of the following solution:

1) In add-source, set the timestamps of newly-added add-source deps to 0 in the timestamp file. This will make us enter reinstallAddSourceDeps on the next build.

2) In reinstallAddSourceDeps, reinstall only those dependencies that are actually installed in the current package DB. This way, we won't automatically install those add-source deps that the user hasn't previously installed manually (with install --dependencies-only or install $PKGID).

3) In install, look at the install plan after a successful installation and update the timestamps of those add-source deps that were installed. This will prevent us from installing add-source deps twice in the following scenario:

$ cabal sandbox add-source /path/to/some-dep
# timestamp of /path/to/some-dep set to 0
$ cabal install --only-dependencies
# Installed some-dep
$ cabal build
# Timestamp of some-dep is 0 and it's already installed - reinstalling

This solution should also work in the presence of multiple package DBs inside the same sandbox (since timestamps are per-compiler).

Mikhail Glushenkov 23Skidoo referenced this issue from a commit in 23Skidoo/cabal May 17, 2013
Mikhail Glushenkov Make newly-added add-source deps override previously installed versions.
Fixes #1197.

This patch is a bit large because it includes several related changes:

1) Remove 'installUseSandbox' from 'InstallFlags' and pass 'useSandbox' as an
additional argument instead.

2) Instead of calling 'reinstallAddSourceDeps' from 'installAction', always pass
'SandboxPackageInfo' to 'install'.

3) Set the timestamps of newly-added add-source deps to 0 in the timestamp file.

4) Move the timestamp file update to 'postInstallActions' from
'withModifiedDeps'. This way, the timestamps are updated even when the user runs
'install --only-dependencies' or 'install some-add-source-dep-package-id'.
649377b
Mikhail Glushenkov 23Skidoo referenced this issue from a commit in 23Skidoo/cabal May 17, 2013
Mikhail Glushenkov Make newly-added add-source deps override previously installed versions.
Fixes #1197.

This patch is a bit large because it includes several related changes:

1) Remove 'installUseSandbox' from 'InstallFlags' and pass 'useSandbox' as an
additional argument instead.

2) Instead of calling 'reinstallAddSourceDeps' from 'installAction', always pass
'SandboxPackageInfo' to 'install'.

3) Set the timestamps of newly-added add-source deps to 0 in the timestamp file.

4) Move the timestamp file update to 'postInstallActions' from
'withModifiedDeps'. This way, the timestamps are updated even when the user runs
'install --only-dependencies' or 'install some-add-source-dep-package-id'.
a646ea8
Mikhail Glushenkov 23Skidoo referenced this issue from a commit in 23Skidoo/cabal May 17, 2013
Mikhail Glushenkov Make newly-added add-source deps override previously installed versions.
Fixes #1197.

This patch is a bit large because it includes several related changes:

1) Remove 'installUseSandbox' from 'InstallFlags' and pass 'useSandbox' as an
additional argument instead.

2) Instead of calling 'reinstallAddSourceDeps' from 'installAction', always pass
'SandboxPackageInfo' to 'install'.

3) Set the timestamps of newly-added add-source deps to 0 in the timestamp file.

4) Move the timestamp file update to 'postInstallActions' from
'withModifiedDeps'. This way, the timestamps are updated even when the user runs
'install --only-dependencies' or 'install some-add-source-dep-package-id'.
4a6af2a
Mikhail Glushenkov 23Skidoo referenced this issue from a commit in 23Skidoo/cabal May 17, 2013
Mikhail Glushenkov Make newly-added add-source deps override previously installed versions.
Fixes #1197.

This patch is a bit large because it includes several related changes:

1) Remove 'installUseSandbox' from 'InstallFlags' and pass 'useSandbox' as an
additional argument instead.

2) Instead of calling 'reinstallAddSourceDeps' from 'installAction', always pass
'SandboxPackageInfo' to 'install'.

3) Set the timestamps of newly-added add-source deps to 0 in the timestamp file.

4) Move the timestamp file update to 'postInstallActions' from
'withModifiedDeps'. This way, the timestamps are updated even when the user runs
'install --only-dependencies' or 'install some-add-source-dep-package-id'.
e8742a5
Mikhail Glushenkov 23Skidoo closed this in #1331 May 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.