Working on a fix for ticket #89

[Stephen Blackheath [to cabal-devel]]

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