Working on a fix for ticket #89
All, I'm working on a fix for ticket #89 (http://hackage.haskell.org/trac/hackage/ticket/89), and I have a specific question for the list: ---- If I do this... build-depends: base Library build-depends: bytestring ...then when the library compiles, it will fail to find the Prelude (i.e. -package base doesn't get passed to ghc). Is there a reason why the global 'build-depends' doesn't add to all targets (i.e. all exes & libs)? That's what I would expect it to do, but that might be just me. Or more to the point, should this be fixed? ---- The code changes I'm working on also fix a couple of other things, so this is the complete list: - Ticket #89: Make it so the executable can depend on the library defined in the same package. If hs-source-dirs is used, you can avoid it compiling the .hs files multiple times. - Make it so build-depends: defined in a Library or Executable block affects only the build of those components, not all components as currently happens. For avoidance of package breakage, this behaviour only happens if you specify cabal-version: >= 1.7 (that is, versions less than 1.7 are entirely excluded). - Make a really simple unit test harness using the test-framework package, and add some test cases for the new behaviour. The idea is that we can gradually integrate existing tests (hunit & quickcheck) into it. - Make UnitTests compile (though hardly any of the tests pass). This patch is not yet tidy enough to submit, but it is complete enough to actually work for GHC. I would welcome any comments. Here it is (against HEAD): http://upcycle.it/~blackh/cabal/cabal-ticket-89-v3.patch Steve
All, Now I'm ready to submit this patch for consideration - I have attached it in 'darcs send' format. Please review. Here it is: http://upcycle.it/~blackh/cabal/cabal-ticket-89-v4.darcs-send It has test cases for the new logic, and some tests for old logic (to make sure it behaves the same on 1.6.0.3 and HEAD). To test: cd tests runhaskell suite Tested on ghc-6.8.2 and ghc-6.10.2 Steve Stephen Blackheath [to cabal-devel] wrote:
All,
I'm working on a fix for ticket #89 (http://hackage.haskell.org/trac/hackage/ticket/89), and I have a specific question for the list:
---- If I do this...
build-depends: base Library build-depends: bytestring
...then when the library compiles, it will fail to find the Prelude (i.e. -package base doesn't get passed to ghc). Is there a reason why the global 'build-depends' doesn't add to all targets (i.e. all exes & libs)? That's what I would expect it to do, but that might be just me.
Or more to the point, should this be fixed? ----
The code changes I'm working on also fix a couple of other things, so this is the complete list:
- Ticket #89: Make it so the executable can depend on the library defined in the same package. If hs-source-dirs is used, you can avoid it compiling the .hs files multiple times.
- Make it so build-depends: defined in a Library or Executable block affects only the build of those components, not all components as currently happens. For avoidance of package breakage, this behaviour only happens if you specify cabal-version: >= 1.7 (that is, versions less than 1.7 are entirely excluded).
- Make a really simple unit test harness using the test-framework package, and add some test cases for the new behaviour. The idea is that we can gradually integrate existing tests (hunit & quickcheck) into it.
- Make UnitTests compile (though hardly any of the tests pass).
This patch is not yet tidy enough to submit, but it is complete enough to actually work for GHC. I would welcome any comments. Here it is (against HEAD):
http://upcycle.it/~blackh/cabal/cabal-ticket-89-v3.patch
Steve
Newer version... http://upcycle.it/~blackh/cabal/cabal-ticket-89-v5.darcs-send I had to fix it so the ability to depend on an internally defined package would fail unless you said 'cabal-version: >= 1.7' - otherwise there's a risk someone will create a package that doesn't work on older versions of cabal than the one they're using. Oops. Steve Stephen Blackheath [to cabal-devel] wrote:
All,
Now I'm ready to submit this patch for consideration - I have attached it in 'darcs send' format. Please review.
Here it is: http://upcycle.it/~blackh/cabal/cabal-ticket-89-v4.darcs-send
It has test cases for the new logic, and some tests for old logic (to make sure it behaves the same on 1.6.0.3 and HEAD). To test:
cd tests runhaskell suite
Tested on ghc-6.8.2 and ghc-6.10.2
Steve
Stephen Blackheath [to cabal-devel] wrote:
All,
I'm working on a fix for ticket #89 (http://hackage.haskell.org/trac/hackage/ticket/89), and I have a specific question for the list:
---- If I do this...
build-depends: base Library build-depends: bytestring
...then when the library compiles, it will fail to find the Prelude (i.e. -package base doesn't get passed to ghc). Is there a reason why the global 'build-depends' doesn't add to all targets (i.e. all exes & libs)? That's what I would expect it to do, but that might be just me.
Or more to the point, should this be fixed? ----
The code changes I'm working on also fix a couple of other things, so this is the complete list:
- Ticket #89: Make it so the executable can depend on the library defined in the same package. If hs-source-dirs is used, you can avoid it compiling the .hs files multiple times.
- Make it so build-depends: defined in a Library or Executable block affects only the build of those components, not all components as currently happens. For avoidance of package breakage, this behaviour only happens if you specify cabal-version: >= 1.7 (that is, versions less than 1.7 are entirely excluded).
- Make a really simple unit test harness using the test-framework package, and add some test cases for the new behaviour. The idea is that we can gradually integrate existing tests (hunit & quickcheck) into it.
- Make UnitTests compile (though hardly any of the tests pass).
This patch is not yet tidy enough to submit, but it is complete enough to actually work for GHC. I would welcome any comments. Here it is (against HEAD):
http://upcycle.it/~blackh/cabal/cabal-ticket-89-v3.patch
Steve
_______________________________________________ cabal-devel mailing list cabal-devel@haskell.org http://www.haskell.org/mailman/listinfo/cabal-devel
On Wed, 2009-05-06 at 11:50 +1200, Stephen Blackheath [to cabal-devel] wrote:
All,
I'm working on a fix for ticket #89 (http://hackage.haskell.org/trac/hackage/ticket/89), and I have a specific question for the list:
---- If I do this...
build-depends: base Library build-depends: bytestring
...then when the library compiles, it will fail to find the Prelude (i.e. -package base doesn't get passed to ghc). Is there a reason why the global 'build-depends' doesn't add to all targets (i.e. all exes & libs)? That's what I would expect it to do, but that might be just me.
It's a bug. When we added the new section style syntax most of the fields that are now put in lib/exe sections were no longer allowed in the global section. For tedious reasons the parser treats build-depends differently from most other fields and so for some reason this field is still allowed in the global section but the value is always ignored.
Or more to the point, should this be fixed?
The simplest fix for now would be to make it an error to have build-depends in the global section.
----
The code changes I'm working on also fix a couple of other things, so this is the complete list:
- Ticket #89: Make it so the executable can depend on the library defined in the same package. If hs-source-dirs is used, you can avoid it compiling the .hs files multiple times.
Ah yes, because ghc will still pick local files over modules from a package.
- Make it so build-depends: defined in a Library or Executable block affects only the build of those components, not all components as currently happens. For avoidance of package breakage, this behaviour only happens if you specify cabal-version: >= 1.7 (that is, versions less than 1.7 are entirely excluded).
Great.
- Make a really simple unit test harness using the test-framework package, and add some test cases for the new behaviour. The idea is that we can gradually integrate existing tests (hunit & quickcheck) into it.
Ok, I'm not familiar with test-framework but from at the description it sounds good
- Make UnitTests compile (though hardly any of the tests pass).
Yeah, that's all the old stuff. For new code I've been adding pure tests to tests/Test/, for example the QC tests for the VersionRange and new VersionIntervals type.
This patch is not yet tidy enough to submit, but it is complete enough to actually work for GHC. I would welcome any comments. Here it is (against HEAD):
Ok, reviewing the v5 version now... Duncan
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
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
participants (2)
-
Duncan Coutts -
Stephen Blackheath [to cabal-devel]