On Tue, 2010-10-12 at 00:30 +0200, Jürgen Nicklisch-Franken wrote:
I did send this mail a week ago or so to the haskell-libraries mailing list, but this was probably the wrong list.
I have in the meantime tested with all cabbal files on haddock, and have fixed some issues.
This patch adds a pretty printer for GenericPackageDescription. It fixes ticket http://hackage.haskell.org/trac/hackage/ticket/647.
Fantastic! Thanks for your contribution.
I added a new module Distribution.PackageDescription.PrettyPrint which exports writeGenericPackageDescription and showGenericPackageDescription. I use the field printers from Parse.hs, so that I had to add export statements for them in Distribution.PackageDescription.Parse.
Right.
I decided to not use a class based approach because: 1. their is already the Text class, which has a parse I didn't want to write, and 2. and more important, I didn't want to use -XFlexibleInstances because I guess cabal should be portable.
Yep.
I have not removed the old printer code, although this may be a good idea (maybe mark it as deprecated first). Then it would make sense as well to rename writeGenericPackageDescription to writePackageDescription and showGenericPackageDescription to showPackageDescription, to be symmetric.
Yes.
I tested with the code from Appendix 1 and all the cabal files on hackage. The test succeds when writing and reading a package description results in an equal description. This is more rigorous then required, cause different GenericPackageDescr.. can have the same semantic, so for example the order of exe definitions is not relevant. But for our needs (GUI editor of cabal files for leksah), e.g. preserving of order is important and it will help to test the parser and printer to make this approach work. Furthermore I want to print out the cabal file as short as possible, e.g. not printing all empty fields like the old printer did.
Right. Preserving order and omitting blank fields makes perfect sense.
Testing the code I found the following problems:
1. Found up an obvious "age old" bug: hunk ./Distribution/PackageDescription/Parse.hs 359 - ghcProfOptions (\val binfo -> binfo{ghcSharedOptions=val}) + ghcSharedOptions (\val binfo -> binfo{ghcSharedOptions=val})
Yes, I actually just fixed this one a week or two ago when I was going through my patch queue. So this change clashes with your patch, but that's the only clash so it's not a bit problem.
2. The order of elements are reversed by the parser. E.g executables: (repos, flags, lib, exes, tests) <- getBody return (repos, flags, lib, exes ++ [(exename, flds)], tests) since the rest is parsed in the line before, the element needs to be prepended. so I changed to: (repos, flags, lib, exes, tests) <- getBody return (repos, flags, lib, (exename, flds): exes, tests) The same applies to test fields.
Fine.
The opposite order problem applies to storeXFields.., which is called in the order of processing, so I changed it to storeXFieldsPD (f@('x':'-':_),val) pkg = Just pkg{ customFieldsPD = (customFieldsPD pkg) ++ [(f,val)]}
Fine.
3. The next problem is that we print nothing for empty fields , which works ok, except for compiler opts, where empty opts differ from not given opts, so I added this line to optsField in ParseUtils, which causes empty opts to be parsed, as if no opts are given: update f opts [] = [(f,opts)]
That line was there already, you added the line: + update _ opts l | and (map null opts) = l update f opts [] = [(f,opts)] update f opts ((f',opts'):rest) | f == f' = (f, opts' ++ opts) : rest
4. The next problem is that we loose final newlines with showFreeText. Astonishing enough the culpid is lines from the List module , which evals lines "a\n" to ["a"]. By the way even the claim that "'unlines' is an inverse operation to 'lines'" from the List module is not true, as unlines (lines "a") -> "a\n" (see Appendix 2). So I use my own unlines' now!.
Yes. I've noticed this before. If only we had had QuickCheck when designing the list module we might have picked more sensible properties.
5. This is connected to the CondTree structure. When I write a Cabl file like: ... library extensions: ForeignFunctionInterface if flag(use_xft) build-depends: X11-xft >= 0.2, utf8-string extensions: ForeignFunctionInterface ... the extension ForeignFunctionInterface in the conditional branch is superflous, as it is specified in any case, so the printer can simplify it to: ... library extensions: ForeignFunctionInterface if flag(use_xft) build-depends: X11-xft >= 0.2, utf8-string ... However the parse result for the two cabal files will differ. Now I gave up here, and added a variable simplifiedPrinting which disables the printer smartness for now. However, I hope someone can make the parser smarter in this respect.
I think it is better to stick to the syntax when pretty printing. If we want to do semantic simplifications that should be done separately. So should simplifiedPrinting be True or False? Currently it says: -- | Recompile with false for regression testing simplifiedPrinting :: Bool simplifiedPrinting = False
6. If free text starts with a line with a dot, the dot must be in a new line.
Right. In future we're going to allow free text without the silly dot stuff. I wonder how we'll handle things then.
7. Version tags have to be printed, which was not the case before (why?).
Actually that was on purpose. We must not print tags as they make no sense at all. Tags are a crazy idea that doesn't work at all. Some old packages use tags and there's not a lot we can do about that, but we have to ignore them or we'll go insane. So I cannot accept this bit of the patch. If you have another suggestion I'm interested to hear. Perhaps we should ignore them when parsing too. Would that work?
8. VersionRangeParens needs to be printed.
Fine. We already have functions to simplify version ranges if we need to do that.
9. Compiler options have to be put in a canonical order after parsing, which is easy as CompilerFlavors do derive Ord.
Why is this? Can we not preserve the order of the original cabal file? Would you like to send in the test code as a patch, to add to the Cabal testsuite? That way we can make sure the round-trip parser/pretty-printer property remains true in future. Ideally we would make the test program use the hackage tarball index and check all of them.
PS. I have added a remark about the new test suite syntax below.
Appendix 3: I find the test suite syntax problematic, for an exe I have to remeber the combination:
test-suite foo type: exitcode-stdio-1.0 main-is: main.hs
, and for a module:
test-suite bar type: library-1.0 test-module: Bar
So its the combination of exitcode-stdio- and main-is or library- and test-module which does the trick. Wouldn't it be easier to write something like:
test-suite foo test-version: 1.0 main-is: main.hs , and for a module: test-suite bar test-version: 1.0 test-module: Bar
You mean you want to make the test type implicit, depending on which fields are specified? I think it's important to specify the test type because there are not necessarily just two. There are two now but the point of the design is to allow for test protocols that we have not yet thought of, as we get more experience with the system. Those new protocols may well reuse the same fields, so the fields used may not uniquely determine the test type. Note that the current Cabal code will remind you of the right fields, depending on the test type that you specify. It also tells you which test types there are. Duncan