Working on a fix for ticket #89

[Duncan Coutts]

On Sat, 2009-05-09 at 16:41 +0000, Duncan Coutts wrote:
> On Wed, 2009-05-06 at 11:50 +1200, Stephen Blackheath [to cabal-devel]
> wrote:

> > http://upcycle.it/~blackh/cabal/cabal-ticket-89-v3.patch
> 
> Ok, reviewing the v5 version now...

Right, so I've got quite a few comments, but it's quite a big patch! I
hope I'm not discouraging you.


So adding targetBuildDepends into the BuildInfo is certainly the way to
go. I'm a bit confused by the changes in the Configuration module and
that there are no changes to the parser.

Here's how I'd expect it to work: adjust the parser so that
build-depends goes into the targetBuildDepends in the BuildInfo. After
parsing we can transform the GenericPackageDescription so that the top
level buildDepends contains the union of the targetBuildDepends. 

Optionally (depending on the cabal-version), we would then change each
targetBuildDepends to contain the top level buildDepends union, rather
than the original. This is the backwards compat hack. Doing it this way,
by changing the data means that we should not need the "if
newPackageDepsBehaviour" stuff when building the package. We can just
follow the data and we get the old behaviour when each component uses
the union of all deps.

I don't see that we need to change anything in the configurations stuff
when resolving GenericPackageDescription to a PackageDescription, though
perhaps I'm missing something.

Why do we have to filter out the internal package deps when saving the
packageDeps in the LocalBuildInfo? Isn't it ok to have them since we'll
really refer to them as packages when we build things.

The need to identify the target all the time when constructing command
lines for ghc etc is a bit unpleasant though perhaps it's essential.
Hmm, yes building one component needs both the original info from
the .cabal file (ie the BuildInfo and PackageDescription) but also the
local post-configure stuff LocalBuildInfo and something similar that's
per-component. It used to be that the only per-component info we needed
was the original BuildInfo but now we need resolved dependencies too.
This'll want refactoring at some point.

Could we make the registering of the local packages more generic. I mean
instead of having each compiler's build function do it, have it done
from the generic build function by calling register (or possibly calling
per-compiler things).

Do you suppose it's possible for us to split these changes into multiple
patches. I'd hope that we can have:
      * package type and parser changes (setting the targetBuildDepends
        and the buildDepends to their union)
      * changes to use the per-component targetBuildDepends (including
        changes in the active build parts)
      * configure changes to switch the behaviour on the cabal version
        (ie using the union vs using correct per-component deps)
Note that so far none of this is to do with internal deps, just allowing
the deps of each component to be different.
      * check for internal deps, disallow for older cabal-version
      * use internal deps during build, register internal packages

Does that look like the dependencies between patches works out? Does it
cover everything?

Perhaps we should try and merge this incrementally. That'd make each bit
easier to review and I could add some refactoring patches as we go
along.

Duncan