Duncan, Now I've got a bit more time, I'll go over everything again just to make sure I've got everything clear. --- Item 1: Combining of dependencies in Old Behaviour (where cabal-versions: contains any versions earlier than 1.7) In the old behaviour, in v1, I resolved each component's [Dependency] to [PackageIdentifier], and merged the result. This was totally wrong and I fixed it in v2 by implementing it like the original code, that is, take the whole-package dependencies from buildDepends, and resolve [Dependency] to [PackageIdentifier] for those. Now, this is simple and we don't need to worry about it any more. --- Item 2: Resolving dependencies to package ids for New Behaviour (where cabal-versions: contains versions >= 1.7 only) Both my v1 and v2 behaved the same for new behaviour. What they did was they took the individual components [Dependencies] and resolved each set to [PackageIdentifier]. I took a look at your implementation, and that works fine for me and certainly solves my problem. Now, as you noted, my implementation used different logic, and I just want to discuss this briefly so we can be sure we get it all right. To summarize: If two components have different dependency version specs for the same dependent package... e.g. Executable one build-depends: network < 2.2.1 Executable two: build-depends: network >= 2.2.0.0 - Your logic says: Combine the specifications and resolve to a single package id, and use it for both components. (e.g. yours would both get network-2.2.0.1.) - My logic says: Treat them separately, because it's quite legitimate for two components in the same package to depend on different versions of the same dependent package. (e.g. 'one' would get network-2.2.0.1 and 'two' would get network-2.2.1.2). The result of my logic would be that the package as a whole could then be considered to depend on multiple versions of network. However, the package can only export one library - and the library is the only part that can be imported by another package. So, when checking for the "This package indirectly depends on multiple versions of the same package." message, you would need to consider only the library component's dependencies. It could be that this adds complexity you don't want. 'cabal-install' would need to look at the union of all components' external dependencies while the "This package indirectly depends..." error message would need to consider the library component's only. That is, they would need a different view from each other of what a package's "external dependencies" are. I thought it was worth mentioning, anyway. --- Item 3: PackageDBStack re-factoring You said:
Looking at part 4, I wonder if instead of passing the internal PackageDB into the buildExe function, we can take this opportunity to finish off the conversion from using PackageDB to PackageDBStack in the LocalBuildInfo and the per-compiler build functions.
I'm all in favour of this, so please go ahead. I'll re-do my part 4 once you've finished. Steve Stephen Blackheath [to cabal-devel] wrote:
Duncan,
Thanks for all your work. I did mean the latter.
My way of viewing it was this: I think it's completely legitimate for different components to pull in different versions of the same package, and that's why I coded it to allow it. However, it isn't legitimate in the old behaviour, because that must be exactly the same as it always has been, and that's what I was referring to.
Well, I won't have time to work on part 4 for another 12 hours from now (and I won't have much time tonight), so please go ahead and re-factor PackageDBStack if you want to do that.
Steve
Duncan Coutts wrote:
Duncan,
I've realized the patch below... (which I pointed you at before) has a slight problem, in that where it tries to replicate the old behaviour, it's merging the dependencies *after* they've been resolved to exact versions. This is actually not equivalent to the old behaviour in the situation where someone specifies different version ranges for the same package in two places. So I'm slightly confused about this problem because I'm not sure if you mean in the code that finalises from GenericPackageDescription to PackageDescription or the bit in configure where it picks actual PackageIds based on the Dependency values from the finalised PackageDescription. I was assuming you meant the latter but if so I
On Fri, 2009-05-29 at 21:42 +1200, Stephen Blackheath [to cabal-devel] wrote: think the bug is still there in v2 of the patch. So that's why I'm confused :-)
I think I've fixed the issue in v2. So now I have it pick deps in the old way and then just select the subset of those needed for each component. This seems to work in the test I tried. I've not re-run the tests you added. Perhaps you'd like to check that.
So I took your patch: * Ticket #89 part 3: Add code to correctly handle internal dependencies.
and split it into two and used the alternative way of selecting per-component deps mentioned above. So it's not a total rewrite, it uses much of the same code. It also includes bits of your part 4 patch, eg the bit to switch behaviour based on the required cabal version. So you'll recognise much of the code as being the same.
So part 4 will need massaging slightly to apply on top of what we've got now. Looking at part 4, I wonder if instead of passing the internal PackageDB into the buildExe function, we can take this opportunity to finish off the conversion from using PackageDB to PackageDBStack in the LocalBuildInfo and the per-compiler build functions. As the name suggests the PackageDBStack allows a whole set of package dbs to be used and then we'd just add the internal package db to the top of that stack.
I can work on the PackageDBStack refactoring first if you prefer.
Duncan