Duncan, Thanks kindly. See below... Duncan Coutts wrote:
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.
We need to get this right. I expected this task to be a pain in the butt, so I am not the least bit discouraged. :)
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.
The way the parser works took a bit of figuring out. Distribution.PackageDescription.Parse.parsePackageDescription returns a GenericPackageDescription structure, which contains 'condLibrary' and 'condExecutables'. These are tree structures of some kind, and they contain all the build-depends information, along with conditionals based on flags defined in the .cabal file. It hasn't been stuffed into the packageDescription yet - that is, buildDepends has not actually been populated yet. This actually happens in Distribution.PackageDescription.Configuration.finalizePackageDescription, right at the top (called from Distribution.Simple.Configure.configure) after some processing on the conditionals. In actual fact, buildDepends probably does get populated by the parser, but its contents are not used before that field gets overwritten in finalizePackageDescription. So, the 'build-depends' field values that the parser returns (in condLibrary and condExecutables fields) are already divided into the different targets (exes & lib). The parser already outputs the needed information and doesn't need to be modified. As currently implemented, the resolution of conditionals - which must take place before buildDepends (and the new targetBuildDepends) can be populated - depends on a "dependency test function" which has knowledge about installed packages, and therefore doesn't belong in the parser. So, there doesn't seem to be much point in modifying the parser.
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.
OK - that's a good idea. I will change it to work that way.
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.
The configuration code currently takes the tree structure with conditionals in it (from GenericPackageDescription), resolves the conditionals against available packages, and uses that to populate buildDepends. In doing so, it discards any association of 'build-depends' constraints with targets - this being the information we now want to keep. So one way or another, the configuration code does need to be modified.
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.
My reasoning was this: There is an "external view" of the package, which is the view used by cabal's "register" functionality. In this view, we don't want to see internal dependencies (because it'll look like a cycle) and we want to see the dependencies for the package as a whole - the union of the individual targets (because we don't care about the package's internal structure). So, I used 'packageDeps' to represent this view, since this is exactly the same view of the package as 'packageDeps' currently has. An alternative would be to get rid of 'packageDeps', and replace it with a function that calculates the "external view" of the package from the targetPackageDeps data. We could give it a more suitable name with a comment that explains this concept of an internal / external view of dependencies. I think that's a much better way. 'packageDeps' is currently used by 1. Haddock 2. cabal's "register" function 3. Makefile generation Part of my motivation in doing it the way I did was to avoid having to modify these.
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.
Perhaps the ideal way would be to separate the targets completely, and have separate buildLib and buildExe functions in the compiler-specific code which only see a target-specific LocalBuildInfo structure. Then we could eliminate the code I added to identify targets.
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).
That would be good. It seems to fits in well with my suggestion above of splitting 'build' into 'buildLib' and 'buildExe'.
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?
That looks good, but like I said before, as things currently stand, no change is needed in the parser. So I want to thrash that one out with you first so I know exactly what I'm doing.
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.
OK - that's a very good way. Since the first step is the addition of targetBuildDepends, let's first get to the bottom of how that's going to be implemented. Then I can work on that first patch. Do you see what I mean that the code needs to be changed in the configuration code, not in the parser? (Except to tidy it up.) ---------- Here is another idea for re-factoring: Currently configure has two steps: 1. (GenericPackageDescription, installed packages) -> finalizePackageDescription -> PackageDescription (with buildDepends now populated). It resolves conditionals and dependencies. (In fact finalizePackageDescription only needs a 'check dependency' function, not the whole list of dependencies.) 2. 'configure': (PackageDescription, installed packages) -> Resolve specific versions of dependencies -> PackageDescription The suggestion is this: - Move the new targetBuildDepends into a new data structure that gets returned by finalizePackageDescription. Get rid of buildDepends and calculate it as necessary (in fact it is only needed inside configure). Now finalizePackageDescription returns additional information instead of a modified PackageDescription. - Move the fields from GenericPackageDescription into PackageDescription. Get rid of GenericPackageDescription. This means that PackageDescription now represents just the output of the parser. The concept of a "finalized" PackageDescription is replaced by a separate data structure. This extra data structure acts as a stepping stone between PackageDescription and LocalBuildInfo. It is only used inside 'configure'. Steve