fpco / inline-c

284 stars 49 forks source link

When compiling cabal, put C files in a subdirectory #30

Closed bitonic closed 7 years ago

bitonic commented 9 years ago

@mboes , I'd like you to take a look of what I'm doing here -- see commit message in dec22a6.

This would fix issue #21 -- I just realized I got the branch name wrong...

mboes commented 9 years ago

So as I've said in #21, I think we should not create another top-level directory that can't easily be cleaned by cabal clean. It's true that it's weird to put stuff in dist/, but actually there's a very nice precedent here, to the point where this is not weird at all by Cabal's current design.

cabal sdist does already include preprocessor output in the tarball by default, provided the output is "platform independent". This is the case of e.g. happy, alex, etc. If you have a happy Foo.x file in your source, cabal sdist will include a dist/build/Foo.hs in the tarball. The reason it does that is because it allows for the user to not have the preprocessor installed.

I think therefore that the simple pragmatic solution going forward is this:

The obligation to list each file explicitly, rather than glob, is not so bad in the end, because while verbose and kind of annoying, it does avoid a bunch of corner cases. In particular, when globbing, if the user renames a module, then the old generated *.c file may be lying around and will be picked up and linked into the resulting binary. Listing the files explicitly at least makes it the user's fault if the wrong c file was picked up.

cc @chpatrick and @mitchellwrosen.

mitchellwrosen commented 9 years ago

+1

bitonic commented 9 years ago

@mboes agreed on the file extension.

I still think that the builddir idea is problematic, compared to referring to the .cabal file, which is going to be more robust.

For example, stack chooses the builddir based on the cabal version, e.g.

--builddir=.stack-work/dist/x86_64-linux/Cabal-1.18.1.5/

The Agda repository is another "notable" repository that uses a builddir based on the version.

In general having to manually specify a path which involves features that are dependent on the current environment seems very cumbersome.

bitonic commented 9 years ago

Obviously there is the option of blindly putting stuff in dist/inline-c or similar, but I'm not sure how convenient that would be. The only advantage I can see is that cabal clean will get rid of it.

mboes commented 9 years ago

Argh, yes, you're right, we can't just assume we know the buildDir. Since we don't have access to it as a variable in the .cabal file, I think the only clean way to do this is to use Setup.hs, which after all was designed for precisely this purpose. It's not that bad either - something like:

import Distribution.Simple
import System.Environment

defaultMainWithInlineC :: [FilePath] -> IO ()
defaultMainWithInlineC extraCSources =
    defaultMainWithHooks simpleUserHooks
      { preConf = \_ _ -> do
          return (Just emptyBuildInfo { cSources = extraCSources }, [])
        confHook = \a b -> do
          lbi <- confHook simpleUserHooks a b
          setEnv "INLINE_C_BUILDDIR" $ buildDir lbi
          return lbi
      }

main = defaultMainWithInlineC
  [ "Data/Foo.c"
  , "Data/Foo/Blah.c"
  ]

We shouldn't be poking holes in Cabal as a build system by leaving cleanup to external scripts that Cabal knows nothing about: it means the Cabal based build system for the package is no longer completely functional and self-contained. Hence the importance of using Setup.hs.

Writing custom hooks is kind of scary, even if it's a copy/paste of just a few lines. But the good news is that in the next version of Cabal, we'll be able to make it not scary at all because Setup.hs will be allowed to list inline-c as an explicit dependency, so we'll be able to define defaultMainWithInlineC once and for all for the user and export it.

mboes commented 9 years ago

In fact we can simplify the hook, since the BUILDDIR can be inferred (it's just the -outputdir arg to GHC):

import Distribution.Simple
import System.Environment

defaultMainWithInlineC :: [FilePath] -> IO ()
defaultMainWithInlineC extraCSources =
    defaultMainWithHooks simpleUserHooks
      { preConf = \_ _ -> do
          return (Just emptyBuildInfo { cSources = extraCSources }, [])
      }

main = defaultMainWithInlineC
  [ "Data/Foo.c"
  , "Data/Foo/Blah.c"
  ]

Cabal can then clean the generated C files properly.

basvandijk commented 8 years ago

I don't have an opinion on the best solution but I just want to say I would really like to have the functionality to separate the generated .c/.cpp files from the hand written Haskell code.

BTW @bitonic thanks for this awesome library! At @LumiGuide we're using it to create an OpenCV 3.0.0 binding.

bitonic commented 8 years ago

Hey Bas!

I'm very happy that you find this library useful.

Technically it's not hard to separate the files, and in fact I had a branch that implemented something along those lines. However I've found it hard to come up with a nice interface for this -- and the current interface works OK. This issue and related PR record some of the annoyances.

If you have input on the kind of interface that you'd like, I'm all ears.

On 26 Nov 2015, at 20:16, Bas van Dijk notifications@github.com wrote:

I don't have an opinion on the best solution but I just want to say I would really like to have the functionality to separate the generated .c/.cpp files from the hand written Haskell code.

BTW @bitonic thanks for this awesome library! At @LumiGuide we're using it to create an OpenCV 3.0.0 binding.

— Reply to this email directly or view it on GitHub.

nh2 commented 8 years ago

@basvandijk Nice! Let us know if/when that'll be available!

lspitzner commented 8 years ago

I am not sure how applicable this is, but it has not been mentioned yet: There is the dist/build/autogen directory where cabal already puts some auto-generated stuff (like Paths_foo.hs). This sounds like the correct place for other auto-generated stuff. My guess would be that the directory is not included by sdist.

ekmett commented 8 years ago

@Ispitzner If you explicitly name stuff in there it'll get included in the sdist regardless. The gl package demonstrates this.

bitonic commented 7 years ago

this is not needed anymore now that we compile the files in through addForeignFile.