Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Two patches for better C support #1080

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

ezyang commented Oct 23, 2012

Resurrected from Irene's (aka dankna) old patchset

Owner

tibbe commented Oct 30, 2012

I don't feel qualified to comment on this change. @dcoutts?

One question: will this slow down builds for packages that contain C sources (like lots of our core packages do)?

Contributor

ezyang commented Oct 30, 2012

Yes it will slow down the build, since we're making an extra invocation of GHC.

Owner

tibbe commented Oct 30, 2012

Can we detect the case where this is needed and only do the extra work then? I'm a bit hesitant to make the already long build times longer.

Contributor

ezyang commented Oct 30, 2012

One possibility is require stub generating Haskell files to be explicitly listed in the Cabal file. It is otherwise a little nontrivial to tell if an HS file will generate a stub; we'd need to look for "foreign export" post pre-processing.

Owner

tibbe commented Oct 30, 2012

I like that approach. It's similar to how you have to list the generated Paths_ if you want to use it.

Member

dcoutts commented Nov 1, 2012

If I've understood correctly (and I've not done a detailed review yet) then I'm not worried about the build time. Doing a separate invocation of ghc to link is cheap, it's not having to load .hi files or anything like that for the link step. Indeed we'll eventually switch to a separate link anyway when we move to doing individual ghc invocations for parallel builds and the like.

So I don't think we should add extra complexity of two different code paths or anything like that.

If you're worried you can time it. I'll be quite surprised if the difference is non-trivial.

ezyang added some commits Oct 22, 2012

@ezyang ezyang Do a no-link build before building C sources, so stub.h headers can b…
…e created.

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>
ef5f8a9
@ezyang ezyang Support for C files as main.
Parts of this patch are derived from Irene Knapp (dankna)'s old
patch set which never made it into Cabal proper.  The basic approach
is to relax Main-is restrictions and do the TH prebuild hack for
C mains as well (so that stub headers are generated.)

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>
157664d
Member

dcoutts commented Dec 10, 2012

Ok, patches modified somewhat and committed as de88f40 02f8eef (with credit given in the commit messages -- BTW, what do people normally do to preserve patch authorship & credit, but when the patches have to be changed and we don't want to have a messy patch history?)

The main difference is that we now always do three steps: compile hs, compile C and link everything. This is simpler and avoids the slightly hacky "pre build" stuff.

@dcoutts dcoutts closed this Dec 10, 2012

Member

23Skidoo commented Dec 11, 2012

BTW, what do people normally do to preserve patch authorship & credit, but when the patches have to be changed and we don't want to have a messy patch history?

The Linux kernel project uses the following procedure (described in more detail here):

  • Each person doing changes to the patch preserves the original commit author (with commit -C or commit --author, or by using commit --amend).
  • Everyone who has edited the patch adds a Signed-Off-By line to the commit message.

Example: torvalds/linux@1bf3751

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment