Add an explicit way of specifying dependencies on the command line #1470

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

idontgetoutmuch commented Sep 4, 2013

e.g.,

--dependency="MyOtherLib=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf30"

If the package names do not match

--dependency="foo=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf30"

then this gives an error

Setup.hs: The following names do match their hash name:
(foo, MyOtherLib)

If the hash is incorrect e.g.

--dependency="foo=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf31"

then this gives an error

Setup.hs: The following dependencies do not exist:
MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf31

Contributor

idontgetoutmuch commented Sep 4, 2013

I should have said I worked on this with Duncan Coutts at ZuriHac. We'd like to generate the --dependency=# in cabal-install but we are waiting on another patch from a parallel ZuriHac project. This change is stand alone so can be added in advance of the parallel change. Dominic aka cinimod.

See also: #1460.

Contributor

Toxaris commented Sep 8, 2013

Is Toxaris@142f4204158190db5a5785f197d3ca6654da4c97 the "other patch" you're waiting for? How should the installed package id (or the whole installed package information) be returned there?

Contributor

idontgetoutmuch commented Sep 9, 2013

I am not sure. Here's the problem we are trying to solve as I understand it:

The solver creates an install plan including installed package ids (as
well as package ids). When cabal-install creates a Setup command, this
does not specify the hashes but only the package and its
version. Although no one cabal database can contain two instances of
the same package with the same version, it is possible to have one
package / version in the global package db and exactly the same
package / version in the user package db. Thus it is possible that the
wrong instance can be used in Setup even though the solver knows
exactly which instance to use.

So we'd like cabal-install to call Setup configure like this:

Using internal setup method with build-type Simple and args:
["configure","--verbose=3",


"--dependency=oeis=oeis-0.3.1-7d35d219f6afbd9ea18a7480f0b8e8b6",
"--dependency=lazysmallcheck=lazysmallcheck-0.6-49a7e994e609104db41293c390f97480",
"--dependency=base=base-4.5.0.0-7aff24101508c8e98d083dc12ec4756b",
"--dependency=QuickCheck=QuickCheck-2.4.2-41cb2884cc20cd78948de62849bd9667",

rather than as is currently done:

Using internal setup method with build-type Simple and args:
["configure","--verbose=3",

"--constraint=oeis==0.3.1",
"--constraint=lazysmallcheck ==0.6",
"--constraint=base==4.5.0.0",
"--constraint=QuickCheck==2.4.2",

That is what my change in cabal allows but cabal-install does not yet generate the information. Is that what your change does?

Contributor

Toxaris commented Sep 9, 2013

My change is just about extracing the installed package id of a newly installed package. This is necessary to build the later packages in an install plan against the correct versions of the earlier packages in an install plan. I guess @dcoutts knows best how all of this fits together.

Member

dcoutts commented Sep 13, 2013

Right, @Toxaris has the other bit we need for the cabal-install side of things. After that it is plumbing to get the InstalledPackageInfo into the InstallPlan for Installed packages. Easiest way I think would be to put the Maybe InstalledPackageInfo into the BuildOk type. That is used already in the Installed case for the PlanPackage.

From there we need to extract the InstalledPackageId for all direct lib deps when about to install the next package. We will then have the invariant that each package that is ready to build depends directly on packages in the PreExisting or Installed state, and Installed ones will always have Just ipkginfo not Nothing because the InstallPlan currently only tracks deps for libs, not tools.

Member

dcoutts commented Sep 13, 2013

So I think the way forward is to merge the Cabal lib parts, and the cabal-install patch from @Toxaris and then we can work on the remaining bits of plumbing.

Contributor

idontgetoutmuch commented Sep 16, 2013

I have done the merge and will now attempt the plumbing

Member

dcoutts commented Sep 18, 2013

Great. Will review.

Contributor

Toxaris commented Sep 19, 2013

With the merge, I think that this pull request now subsumes #1462.

There's a comment on Toxaris/cabal@142f4204158190db5a5785f197d3ca6654da4c97. Maybe @idontgetoutmuch can adress it? I cannot work on it the next couple of days (weeks?). Sorry.

Member

dcoutts commented Oct 17, 2013

Ok, I think we should get this stuff reviewed and get it in.

We can start by merging the two patches in #1462. Then d565558 looks ok, though I think much of that code that is currently inline in configure could be pulled out into a separate function, and with a few more comments about what it is doing. Error messages could be improved a tad too I think.

So we can start there. After that, @Toxaris, could you update your 142f420 patch, including the review comments. @idontgetoutmuch when that's done, could you rebase your final cabal-install patch and confirm it's all working?

(@23Skidoo has said he'll help review too. Ta!)

Contributor

idontgetoutmuch commented Oct 19, 2013

@dcoutts ok I will amend d565558 as you suggest. I already merged the patch by @Toxaris (142f420). I guess I just do this again when the updated patch is ready.

Contributor

idontgetoutmuch commented Oct 20, 2013

This still needs to be fixed of course:

~/deleteMe $ /Users/dom/Library/Haskell/ghc-7.6.2/lib/cabal-install-1.19.0/bin/cabal install --reinstall groupoids --cabal-lib-version=1.16.0
Resolving dependencies...
In order, the following will be installed:
groupoids-4.0 (reinstall)
Warning: Note that reinstalls are always dangerous. Continuing anyway...
[1 of 1] Compiling Main             ( /var/folders/3p/m593dprn5snbjz6sc45c77f80000gn/T/groupoids-4.0-83111/groupoids-4.0/dist/dist-sandbox-e83b2380/setup/setup.hs, /var/folders/3p/m593dprn5snbjz6sc45c77f80000gn/T/groupoids-4.0-83111/groupoids-4.0/dist/dist-sandbox-e83b2380/setup/Main.o )
Linking /var/folders/3p/m593dprn5snbjz6sc45c77f80000gn/T/groupoids-4.0-83111/groupoids-4.0/dist/dist-sandbox-e83b2380/setup/setup ...
unrecognized option `--dependency=semigroupoids=semigroupoids-4.0-76c822e325907d2c647f10c0cfdb0b53'

unrecognized option `--dependency=base=base-4.6.0.1-2514ecbfe6573e639515d3e91d6e1f29'
Failed to install groupoids-4.0
cabal: Error: some packages failed to install:
groupoids-4.0 failed during the configure step. The exception was:
ExitFailure 1
Contributor

idontgetoutmuch commented Oct 20, 2013

I now have a) a patch for cabal so that it understands --dependency and installs the exact hash and b) a patch for cabal-install to generate --dependency. I am not sure what to do next. Should I submit them as two separate pull requests? Do I have to delete my existing pull request and resubmit my amended patch (a) so that @dcoutts and @23Skidoo can review? And in parallel submit my patch (b)?

Member

23Skidoo commented Oct 20, 2013

@idontgetoutmuch

Do I have to delete my existing pull request and resubmit

No need to do that, you can just use git push -f to update this pull request destructively.

Member

23Skidoo commented Oct 24, 2013

Also, I think that both of these changes should logically go in a single pull request.

idontgetoutmuch and others added some commits Sep 4, 2013

@idontgetoutmuch idontgetoutmuch Add an explicit way of specifying dependencies on the command line
e.g.,

--dependency="MyOtherLib=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf30"

If the package names do not match

--dependency="foo=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf30"

then this gives an error

Setup.hs: The following names do match their hash name:
(foo, MyOtherLib)

If the hash is incorrect e.g.

--dependency="foo=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf31"

then this gives an error

Setup.hs: The following dependencies do not exist:
MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf31
1a37a47
@Toxaris @idontgetoutmuch Toxaris Capture installed package information.
The goal is to learn the installed package id of the package we just
installed, as necessary for #1860. We achieve this by inserting an
additional call to "setup register" that produces the installed package
information in a file. We read and parse that file and could now return
the installed package id, but it is not clear what interface would be
appropriate.
46f8885
@idontgetoutmuch idontgetoutmuch Create hashes from the solver install plan and pass these to cabal so
that the exact packages are installed rather than letting cabal choose
for itself (potentially choosing the package with the incorrect
hash). Closes: haskell#1460.
6b6912c
@idontgetoutmuch idontgetoutmuch Refactor the patch to add an explicit way of specifying dependencies
on the command line so that the logic is in a separate function.
9f4ee3a
@idontgetoutmuch idontgetoutmuch Rename functions for easier reading. 522cd36
@idontgetoutmuch idontgetoutmuch Don't use the new flag with old cabals: 1401b52
Contributor

idontgetoutmuch commented Oct 25, 2013

Ok I believe I have updated the pull request so it is ready for review. Fiat justitia ruat caelum!

Member

23Skidoo commented Oct 25, 2013

@idontgetoutmuch Great, thanks. Will take a look soon.

idontgetoutmuch and others added some commits Sep 4, 2013

@idontgetoutmuch @dcoutts idontgetoutmuch Add an explicit way of specifying dependencies on the command line
e.g.,

--dependency="MyOtherLib=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf30"

If the package names do not match

--dependency="foo=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf30"

then this gives an error

Setup.hs: The following names do match their hash name:
(foo, MyOtherLib)

If the hash is incorrect e.g.

--dependency="foo=MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf31"

then this gives an error

Setup.hs: The following dependencies do not exist:
MyOtherLib-1.0-073259a42bbf95f818c899c57ba5bf31
caba878
@dcoutts dcoutts Rearange and simplify the --dependency configure code a bit
Use a map from package name rather than from constraint, for the
info on the specific packages to use.
6002f62
@Toxaris @dcoutts Toxaris Capture installed package information.
The goal is to learn the installed package id of the package we just
installed, as necessary for #1860. We achieve this by inserting an
additional call to "setup register" that produces the installed package
information in a file. We read and parse that file and could now return
the installed package id, but it is not clear what interface would be
appropriate.
2b10869
@idontgetoutmuch @dcoutts idontgetoutmuch Create hashes from the solver install plan and pass these to cabal so
that the exact packages are installed rather than letting cabal choose
for itself (potentially choosing the package with the incorrect
hash). Closes: haskell#1460.
5e46978
@dcoutts dcoutts Opinionated tweaking of the previous patch 44a2ced
@idontgetoutmuch @dcoutts idontgetoutmuch Don't use the new flag with old cabals: 9af2f51
@dcoutts dcoutts And don't use the old flag with new cabals
We generate both --constraint and --dependency flags, but we only need
to pass one or the other, depending on the Cabal version.
7e88be5
@dcoutts dcoutts Bump version to 0.19.1
Since we now have a new version-dependent feature: the new --dependency
configure flag.
adb22d7
Member

dcoutts commented Oct 25, 2013

Ok, I've reviewed the patches and hacked them around a bit. Here's my changes:

https://github.com/haskell/cabal/compare/installed-packageids

@idontgetoutmuch @23Skidoo if either of you could check this still work that'd be great :-)

Member

23Skidoo commented on adb22d7 Oct 25, 2013

LGTM.

Member

23Skidoo commented on 7e88be5 Oct 25, 2013

LGTM.

Member

23Skidoo commented on 9af2f51 Oct 25, 2013

LGTM.

Oh, we should fix this to fail with a sensible error. This case would mean that the Setup.hs gave us a bogus package info file. That could easily happen with a Custom Setup.hs, so we ought to have a human comprehensible error.

Why not make this fragment a helper function? E.g. maybePkgConf <- maybeRegister ...

Member

23Skidoo commented on 2b10869 Oct 25, 2013

LGTM with minor comments.

Member

23Skidoo commented on 6002f62 Oct 25, 2013

LGTM.

Member

23Skidoo commented on caba878 Oct 25, 2013

LGTM.

Member

23Skidoo commented on 44a2ced Oct 25, 2013

LGTM.

Member

23Skidoo replied Oct 25, 2013

But see my comment about ConfiguredPackage. This could also simplify the type of InstallPlan.ready.

Member

dcoutts replied Oct 29, 2013

The reason we cannot make it part of ConfiguredPackage is about when that information becomes available.

The InstalledPackageId is not determined up front by cabal-install, but by the Setup.hs when we ask it to register (or to give us the reg info). However by that time the ConfiguredPackage has already been created and used as part of the InstallPlan. The InstallPlan creates lots of these ConfiguredPackages using dependencies on source package ids, implicitly referring to the package instances within the InstallPlan.

s/we something/we do something/

Instead of passing deps as an argument, shouldn't they be made a part of ConfiguredPackage?

Member

23Skidoo commented on 5e46978 Oct 25, 2013

LGTM.

Member

23Skidoo commented Oct 25, 2013

@dcoutts Looks good to me in general, with minor comments.

I had trouble parsing this. What does "finalise function expects constraints" mean?

Member

23Skidoo replied Oct 26, 2013

"finalise function" = "function that performs finalisation"

Contributor

idontgetoutmuch commented Oct 26, 2013

@dcoutts One minor comment.

Contributor

idontgetoutmuch commented Oct 26, 2013

How do I get the @dcoutts' changes in my local repo? I think

git pull upstream installed-packageids
Contributor

idontgetoutmuch commented Oct 26, 2013

A minor question: why is () preferred to $?

@idontgetoutmuch idontgetoutmuch Merge branch 'installed-packageids' of https://github.com/haskell/cabal
Conflicts:
	Cabal/Distribution/Simple/Configure.hs
	cabal-install/Distribution/Client/Install.hs
	cabal-install/Distribution/Client/InstallPlan.hs
	cabal-install/Distribution/Client/Setup.hs
5edb41a
Contributor

idontgetoutmuch commented Oct 26, 2013

In a sandbox I now get

~/deleteMe $ /Users/dom/Library/Haskell/ghc-7.6.2/lib/cabal-install-1.19.0/bin/cabal install groupoids
Resolving dependencies...
cabal: InstallPlan: internal error: configured package depends on a non-library package

Not sure what is broken. Can anyone reproduce? Possibly my merge (above) was incorrect. I need to work on my presentation now so maybe I will be able to look next week.

Contributor

idontgetoutmuch commented Oct 26, 2013

afaics my merge is correct: idontgetoutmuch/cabal@haskell:installed-packageids...master so I am not sure what is broken.

Member

23Skidoo commented Oct 26, 2013

@idontgetoutmuch Can you please clean up the history and remove the merge commits?

Contributor

idontgetoutmuch commented Oct 26, 2013

@23Skidoo I'm not really sure what that means. Do you mean undo this change: idontgetoutmuch/cabal@5edb41a? In which case how do I get @dcoutts' changes into my local repo? Also with the latest changes, cabal no longer works - see above. I believe it was working at 1401b52. Not sure what to do about that as I don't really have much time this week.

Member

23Skidoo commented Oct 26, 2013

@idontgetoutmuch If you don't have any recent changes on your branch, you can just delete it, recreate it from @dcoutts's version and then do push -f. Otherwise you may need to rebase.

Member

23Skidoo commented Oct 26, 2013

It's better not to have merge commits in pull requests as they make it harder to understand what is going on.

Member

23Skidoo commented Oct 27, 2013

@dcoutts

I've pushed some patches to the installed-packageids branch. It looks like it's not possible to change the type of the deps field in ConfiguredPackage to [InstalledPackageInfo], but maybe we can make introduce a new type for representing packages that are not yet installed but whose dependencies are?

Contributor

Toxaris commented Oct 28, 2013

This sounds related to the Manifest type @copton was working on at ZuriHac. I don't have his code, but he told me in an email:

Assume the following interface:

scanDirectory :: PathConvention -> FilePath -> IO [FilePath]
Manifest :: Maybe InstalledPackageId -> ConfiguredProgram -> PackageDatabase -> [FilePath]-> Manifest
installByManifest :: Verbosity -> PathConvention -> Manifest -> IO ()
uninstallByManifest :: Verbosity -> Manifest -> IO ()
saveManifest :: Manifest -> FilePath -> IO ()
loadManifest :: FilePath -> IO Manifest

The idea was to put together a manifest that contains all information about how to install and register a package, and then call installByManifest. The cool new thing would then be to save the manifest on disk and be able to call uninstallByManifest later to undo the installation. Maybe the "new type for representing packages that are not yet installed but whose dependencies are" should be (or contain) a manifest?

Member

23Skidoo commented Oct 28, 2013

@Toxaris I had something much simpler in mind: a version of ConfiguredPackage with deps :: [PackageId] replaced by deps :: [InstalledPackageInfo].

Contributor

Toxaris commented Oct 29, 2013

@23Skidoo Yes that makes sense for the changes discussed here. I just wanted to mention the possible interaction with this other work, which might also affects what information is stored where.

Member

dcoutts commented Oct 29, 2013

Yes it'd be possible to have a variant of ConfiguredPackage that uses installed package ids rather than source package ids for deps, but it's not clear it buys us much. As soon as packages get into that state then they are candidates to build.

Member

23Skidoo commented Oct 29, 2013

@dcoutts

Yes it'd be possible to have a variant of ConfiguredPackage that uses installed package ids rather than source package ids for deps, but it's not clear it buys us much.

That'd simplify the code a little bit, that's all.

As soon as packages get into that state then they are candidates to build.

Right, so we could make InstallPlan.ready return a ReadyToBuildPackage instead of ConfiguredPackage + a [InstalledPackageInfo].

Member

dcoutts commented Oct 29, 2013

@idontgetoutmuch

configured package depends on a non-library package

Ok, that's interesting and clearly a problem. Can you give me details to reproduce that? We can also improve the error message (at least for debugging purposes) to list the two packages involved.

It comes from this bit of code:

isInstalledDep :: PackageIdentifier -> Maybe Installed.InstalledPackageInfo
isInstalledDep pkgid =
  case PackageIndex.lookupPackageId (planIndex plan) pkgid of
    Just (PreExisting (InstalledPackage instPkg _)) -> Just instPkg
    Just (Installed _ (BuildOk _ _ (Just instPkg))) -> Just instPkg
    Just (Installed _ (BuildOk _ _ Nothing))        -> internalError depOnNonLib
    ...
depOnNonLib = "configured package depends on a non-library package"

I don't yet see what is wrong with the logic there. In that error case we've just found a package depending on a package that has been successfully installed but it actually provides no library. I thought that could not happen. Can it? Or is there something else I'm missing?

Member

dcoutts commented Oct 29, 2013

Ahh! It's the --dry-run printer. It uses Nothing for everything. Ok...

Member

dcoutts commented Oct 29, 2013

@idontgetoutmuch Fixed I think ef34404.

Member

dcoutts commented Oct 29, 2013

@23Skidoo Yes, we could add an extra state for packages ready to build. I'm happy to have that as an independent change though.

dcoutts referenced this pull request Oct 30, 2013

Merged

Installed packageids #1567

Member

dcoutts commented Oct 30, 2013

See the new pull req in #1567

23Skidoo closed this Oct 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment