One thing is not quite right with my ticket 89 patch

[Stephen Blackheath [to cabal-devel]]

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:
>> On Fri, 2009-05-29 at 21:42 +1200, Stephen Blackheath [to cabal-devel]
>> 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
>> 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
>>
>>
>