Fix c2hs handling in Cabal #6233

vmchale · 2019-09-11T03:55:14Z

This fixes a 13 year-old bug, viz. #55 aka #1906

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.
If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

I tested it by building my own library.

…of bootstrap script

phadej · 2019-09-11T12:34:29Z

cabal-install/bootstrap.sh

            echo "Cabal-${CABAL_VER} will be installed from the local Git clone."
-            (cd ../Cabal && install_pkg ${CABAL_VER} ${CABAL_VER_REGEXP})
+            (cd ../Cabal && ${ALEX_PATH} -g Distribution/C2Hs/Lexer.x && install_pkg ${CABAL_VER} ${CABAL_VER_REGEXP})


I'm on the fence with these. The lexer for .cabal files is generated in manual preprocessing step (see Makefile). And rather keep it that way, i.e. let's not have compile-time dependency on alex.

phadej · 2019-09-11T13:36:29Z

Makefile

@@ -3,6 +3,7 @@
 .PHONY : cabal-install-dev cabal-install-prod

 LEXER_HS:=Cabal/Distribution/Fields/Lexer.hs
+LEXER_CHS:=Cabal/Distribution/C2Hs/Lexer.hs


We still need to commit in Lexer.hs file too. So the people who do not modify Lexer.x shouldn't need to be aware of it.

Should be done.

vmchale · 2019-09-12T22:59:32Z

CI is acting up, but this should be ready to merge.

phadej · 2019-09-13T09:11:47Z

Cabal/Distribution/C2Hs.hs

+{-# LANGUAGE RankNTypes #-}
+{-# LANGUAGE FlexibleContexts #-}
+
+-- Based on https://github.com/gtk2hs/gtk2hs/blob/master/tools/src/Gtk2HsSetup.hs#L414


That's GPLd repository, we cannot "base" anything on that code.

Oh, good point.

phadej · 2019-09-13T09:14:04Z

boot/C2HsLexer.x

+
+$module = [A-Za-z\.]
+
+tokens :-


This file needs comments, what it does. Is there a reason why you chose alex, and not writing something using parsec?

I used alex because it seems to be the best choice for lexers and I've used it in the past for identifying dependencies in e.g. C.

Well, I don't agree that we need to write any new alex code. It's write-only for almost any other contributor. You should been asked before making tech choice

phadej · 2019-09-13T09:15:11Z

Cabal/Distribution/C2Hs.hs

+import Distribution.Simple.Utils (dieNoVerbosity, findFileWithExtension, withUTF8FileContents)
+import System.FilePath (joinPath)
+
+reorderC2Hs :: [FilePath] -> [ModuleName] -> IO [ModuleName]


This need a proper comment, what it does, and why.

phadej · 2019-09-13T09:18:02Z

Cabal/Distribution/C2Hs.hs

+  mods <- case getImports con of
+        Right ms -> case traverse simpleParsec ms of
+            Just ms' -> pure ms'
+            Nothing -> dieNoVerbosity ("Cannot parse module name in c2hs file " ++ f)


This dieing here feels wrong. There should be way to fail in pure way...

We need tests.

I have changed the dieNoVerbosity to a warn.

I've added some tests to ensure it works as I need it to.

phadej · 2019-09-13T09:19:00Z

Cabal/Distribution/C2Hs.hs

+        Left err -> dieNoVerbosity ("Cannot parse c2hs import in " ++ f ++ ": " ++ err)
+  pure (md { moduleRequires = mods })
+
+sortTopological :: [ModDep] -> [ModDep]


Is there a way to Distribution.Compat.Graph to not reinvent the wheel?

Yes; I've fixed this.

phadej · 2019-09-13T09:20:08Z

Cabal/Distribution/Simple/PreProcess.hs

@@ -168,7 +169,8 @@ preprocessComponent pd comp lbi clbi isSrcDist verbosity handlers = do
  (CLib lib@Library{ libBuildInfo = bi }) -> do
    let dirs = hsSourceDirs bi ++ [autogenComponentModulesDir lbi clbi
                                  ,autogenPackageModulesDir lbi]
-    for_ (map ModuleName.toFilePath $ allLibModules lib clbi) $
+    mods <- reorderC2Hs dirs (allLibModules lib clbi)


Why only for librayr components, shouldn't this be done for all components?

Yes, this should be fixed now.

vmchale · 2019-09-26T17:36:14Z

@phadej is this ready to be merged?

phadej

I'm sorry to say no to this.

There are issues I commented inline. Also I'd personally expect a single comment with a good commit message explaining what and why that commit does. There're plenty of code.

But I'm not familiar with issue #55, and maybe someone talked you already to do this PR this way.

Please find someone else to review this. as said, I don't feel this is right, but I cannot explain myself better.

If someone else want to take responsibility of merging this, I won't be offended.

phadej · 2019-09-26T21:48:41Z

boot/C2HsLexer.x

+
+$module = [A-Za-z\.]
+
+tokens :-


Well, I don't agree that we need to write any new alex code. It's write-only for almost any other contributor. You should been asked before making tech choice

phadej · 2019-09-26T21:50:55Z

Cabal/tests/C2Hs/src/Codec/Archive/Types/Foreign.chs

@@ -0,0 +1,119 @@
+{-# LANGUAGE DeriveAnyClass #-}
+{-# LANGUAGE DeriveGeneric  #-}


Is this file actually compiled, or is it only an input for tests?

Can we place it's somewhere where it doesn't look like real file.

I value that we have "real life" tests inputs, but we also should have something more minimal.

Also negative examples.

23Skidoo · 2019-09-27T02:25:37Z

/cc @dcoutts

vmchale · 2019-09-28T19:20:16Z

@hvr thoughts?

I'm quite keen on getting this merged (/fixed), and I'm open to rearranging most of it based on the experience of others (I am quite partial to the alex-based lexer, though).

vmchale · 2019-09-28T19:40:52Z

(Also, if someone else wants to rewrite the lexer I would not be offended - but I would not know how to do this without alex or a similar tool/library for lexers)

DanielG · 2019-10-05T11:44:31Z

@vmchale I'm also interested in improving dependency tracking for cabal preprocessors, but in general, not just for c2hs. I don't really understand in detail what exactly c2hs needs here, maybe you cain explain what is going on?

Also is there a reason you can see that would prevent doing this more generally, by say adding an eplicit interface for any preprocessors to communicate dependencies to Cabal instead of Cabal having to parse the preprocessor's input which seems like a pretty nasty violation of the separation of concerns?

vmchale · 2019-10-08T18:56:07Z

@DanielG That's a good point. It might be better to expose a general interface to allow preprocessors to reorder dependencies.

As for what c2hs needs here: I think the easiest way to explain is an example: https://github.com/vmchale/libarchive/blob/master/src/Codec/Archive/Foreign/Archive.chs#L327

Briefly, c2hs can {# import #} another .chs module which gives that module some "context" for marshaling Haskell types.

If you have any more questions about the specific c2hs case, feel free to ask.

My 2¢ is that Cabal should include a special case for c2hs, since it tries to includes support for .chs files out of the box.

DanielG · 2019-10-09T08:04:05Z

I'm not against shipping special support code for the builtin preprocessors, I just think it should be possible for external ones to work to the same extent. Possibly with some custom Setup.hs code but ideally I'd like to just have some protocol that preprocessors can choose to expose which Cabal can automatically use to give you improved dependency tracking.

vmchale · 2019-10-13T00:05:05Z

Right, my guess is it's easier to require custom Setup.hs code - designing such a protocol is more complex.

phadej · 2019-11-22T10:39:27Z

I have thinking about this, and I won't accept this approach. There are two options:

Either c2hs is tought a variant where Cabal can pass it all the files to process
Or there is some discovery option, so Cabal could ask c2hs (again passing all the files) in which order to process them.

cf. that with ghc --make or possible single-shot compilation with initial dependency discovery.

To say it directly: I won't accept cabal "learning" about the contents of source files. c2hs might be quite stable, but there's no reason to assume it won't change. Let's not couple c2hs and Cabal.

vmchale · 2019-11-26T15:55:07Z

@phadej So you think it would be sensible to provide build tools a mechanism to pass dependency reorderings to cabal-install?

Should a spec be decided on on the cabal side before making PRs to c2hs and so on?:

Kleidukos · 2023-05-17T06:31:59Z

Marking this PR as draft 🙂

vmchale added 12 commits September 10, 2019 21:33

Add sorting mechanism for chs files

fbef0e3

Fix nested parser bug + remove dead code

c3e9b87

Style improvements + slimming

e74115e

Reorder c2hs files in a library

cca903b

Prettify

2290218

Update ChangeLog

625423e

Compatibility with older GHCs

71b6d32

Disbale warning since we're using a wrapper/template

16b00a2

Use Cabal prelude

8358fdf

Fix build for older GHCs

6338732

Check Cabal/Distribution/C2Hs/Lexer.hs into version control for sake …

c018b57

…of bootstrap script

Use bootstraph script for Alex

f721560

phadej reviewed Sep 11, 2019

View reviewed changes

vmchale added 2 commits September 11, 2019 07:40

Use Makefile for lexer

64c7d1a

Use makefile approach

6281535

phadej reviewed Sep 11, 2019

View reviewed changes

vmchale added 3 commits September 11, 2019 08:44

Modify makefile

bc1ed2c

Try to reduce time on Mac

0dc1c71

Travis don't install happy

a4a2448

phadej reviewed Sep 13, 2019

View reviewed changes

phadej suggested changes Sep 13, 2019

View reviewed changes

vmchale added 6 commits September 13, 2019 11:39

Pass verbosity appropriately + use cabal's graph/sort capabilities

f71b5af

Add comments to functions

a8c95ae

Clean up + consistent indentation

5f2814f

Polish up lexer module w.r.t. documentation

746ae57

Use revTopSort instead of reverse . topSort

2154c2c

Merge branch 'master' of github.com:vmchale/cabal

eb3d592

vmchale added 3 commits September 15, 2019 23:23

Merge branch 'master' of github.com:haskell/cabal

280e647

Merge branch 'master' of github.com:haskell/cabal

f4c354a

Merge branch 'master' of github.com:haskell/cabal

d6f6feb

vmchale mentioned this pull request Sep 26, 2019

Bump cabal-version allowed in .cabal files #6261

Closed

4 tasks

phadej suggested changes Sep 26, 2019

View reviewed changes

vmchale added 3 commits September 28, 2019 14:10

Fix block comments

6b461de

Add unit tests for block comments

6ae37b0

Move test modules

fe760c5

vmchale added 2 commits September 28, 2019 14:33

Expand module-level comment

6031b43

Add a module-level comment to Cabal.Distribution.C2Hs

ca7537b

vmchale mentioned this pull request Oct 8, 2019

Allow preprocessor-specific reorderings #6291

Closed

4 tasks

Merge branch 'master' of github.com:haskell/cabal

5bc4c03

merge

0bba20f

phadej mentioned this pull request Apr 22, 2020

Supporting BNFC #6726

Open

vmchale mentioned this pull request May 30, 2021

Pp order register #7412

Merged

3 tasks

gbaz added the blocked: decision-needed label Aug 12, 2021

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022

ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022

Kleidukos marked this pull request as draft May 17, 2023 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix c2hs handling in Cabal #6233

Fix c2hs handling in Cabal #6233

vmchale commented Sep 11, 2019

phadej Sep 11, 2019

vmchale Sep 11, 2019

phadej Sep 11, 2019

vmchale Sep 11, 2019

vmchale commented Sep 12, 2019

phadej Sep 13, 2019

vmchale Sep 13, 2019

phadej Sep 13, 2019

vmchale Sep 13, 2019

phadej Sep 26, 2019

phadej Sep 13, 2019

vmchale Sep 13, 2019

phadej Sep 13, 2019

vmchale Sep 13, 2019

phadej Sep 13, 2019

vmchale Sep 14, 2019

phadej Sep 13, 2019

vmchale Sep 14, 2019

vmchale commented Sep 26, 2019

phadej left a comment

phadej Sep 26, 2019

phadej Sep 26, 2019

23Skidoo commented Sep 27, 2019

vmchale commented Sep 28, 2019

vmchale commented Sep 28, 2019

DanielG commented Oct 5, 2019

vmchale commented Oct 8, 2019

DanielG commented Oct 9, 2019

vmchale commented Oct 13, 2019

phadej commented Nov 22, 2019

vmchale commented Nov 26, 2019

Kleidukos commented May 17, 2023

		@@ -0,0 +1,119 @@
		{-# LANGUAGE DeriveAnyClass #-}
		{-# LANGUAGE DeriveGeneric #-}


		$module = [A-Za-z\.]

		tokens :-


		$module = [A-Za-z\.]

		tokens :-

Fix c2hs handling in Cabal #6233

Are you sure you want to change the base?

Fix c2hs handling in Cabal #6233

Conversation

vmchale commented Sep 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmchale commented Sep 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmchale commented Sep 26, 2019

phadej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

23Skidoo commented Sep 27, 2019

vmchale commented Sep 28, 2019

vmchale commented Sep 28, 2019

DanielG commented Oct 5, 2019

vmchale commented Oct 8, 2019

DanielG commented Oct 9, 2019

vmchale commented Oct 13, 2019

phadej commented Nov 22, 2019

vmchale commented Nov 26, 2019

Kleidukos commented May 17, 2023