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:
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