New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow preprocessors to specify extra C sources #2467

Merged
merged 1 commit into from Mar 29, 2015

Conversation

Projects
None yet
2 participants
@ian-ross
Copy link
Member

ian-ross commented Mar 13, 2015

Alter return type from preprocessing to allow preprocessors like hsc2hs and C2HS to inform Cabal of extra C sources that they create that need to be compiled and linked. I believe this fixes #238 as well as providing equivalent support for a related feature in new versions of C2HS where the preprocessor generates some C wrapper functions to help with argument marshalling.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 13, 2015

This breaks the build on Travis.

Please add a changelog note. Maybe also a test and some documentation?

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 13, 2015

@23Skidoo Will do. I hadn't seen the warnings that are breaking the Travis build. And where should documentation go? Just inline comments, or is there a better place? (Sorry if that's a dumb question: this is the first time I've contributed to Cabal...)

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 14, 2015

You probably compiled without -Wall. Documentation for user-facing features lives under Cabal/doc.

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 14, 2015

@23Skidoo I think this is now better -- there's a test (that helped fix a problem...), a changelog comment, some documentation and the build on Travis is fine. What's your preferred approach for merging PRs with extra commits? Should I squash the commits into one and force push, close the PR and open another one with a squashed commit, or just leave it as is?

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 14, 2015

Should I squash the commits into one and force push, close the PR and open another one with a squashed commit, or just leave it as is?

If you want to squash the commits, you can just do push -f after rebase -i. The PR will be updated. This sometimes makes code review comments attached to old diffs (in this case there aren't any) to vanish, though.

@ian-ross ian-ross force-pushed the ian-ross:preprocessor-extra-sources branch from 70acd5f to 0af5a23 Mar 14, 2015

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 14, 2015

OK, rebased and pushed.

@@ -333,7 +333,9 @@ externalSetupMethod verbosity options pkg bt mkargs = do
srcNewer <- src `moreRecentFile` setupHs
when srcNewer $ if useHs
then copyFileVerbose verbosity src setupHs
else runSimplePreProcessor ppUnlit src setupHs verbosity
else do

This comment has been minimized.

@23Skidoo

23Skidoo Mar 15, 2015

Member

Isn't this do-block equivalent to void $ runSimplePreProcessor ...?

This comment has been minimized.

@ian-ross

ian-ross Mar 15, 2015

Member

Oops. Yes. I'll fix it.

where inFile = normalise (inBaseDir </> inRelativeFile)
outFile = normalise (outBaseDir </> outRelativeFile)

mkLessSimplePreProcessor :: (FilePath -> FilePath -> Verbosity -> IO [FilePath])

This comment has been minimized.

@23Skidoo

23Skidoo Mar 15, 2015

Member

Not a very good name, IMO. Maybe add a comment explaining why it is "less simple".

This comment has been minimized.

@ian-ross

ian-ross Mar 15, 2015

Member

It's a terrible name. It was supposed to be a placeholder while I thought of a better name... I'll change it and add a comment.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 15, 2015

LGTM with minor comments.

This changes the PreProcessor type, and by extension PPSuffixHandler, which is part of UserHooks. @dcoutts, are we OK with this?

@ian-ross ian-ross force-pushed the ian-ross:preprocessor-extra-sources branch from 0af5a23 to df695ae Mar 15, 2015

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 16, 2015

So the main problem here is that this change breaks Setup scripts that define custom preprocessors. See here for an example of such setup script.

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 16, 2015

@23Skidoo OK, let me think of a way to do this in a way that's backwards compatible with the existing PreProcessor types. I didn't really take on board that that stuff was user-visible.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 16, 2015

This may be quite hard given that the UserHooks API is not really designed for extensibility. If we ever do a Cabal 2.0 release, we should take the opportunity to hide the internal structure of all user-visible records.

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 16, 2015

Yeah, I was just looking at that. It's going to be hard to do this, because the PreProcessor type is exported as PreProcessor(..) -- I changed mkSimplePreProcessor in a way that would work with that example you linked to, but if people can construct arbitrary PreProcessor instances, I can't see how to deal with a change in the type of runPreProcessor in a backwards compatible way.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 16, 2015

There should at least be a way to write setup scripts that are compatible with both old and new versions of Cabal. It'd be also nice to check whether this change breaks any currently existing package on Hackage.

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 16, 2015

That makes sense. Let me collect some data: I'll look for all the packages on Hackage that use custom build scripts and see if any of them are broken by this change. I'll also collect all the examples of custom preprocessors I can find. That won't help with peoples' private code that's not on Hackage, but at least it's a start and should give some idea of how this is used in practice. Results in a couple of days...

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 16, 2015

Cool, thank you for being so thorough.

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 19, 2015

OK, this would break all the Gtk packages. Don't see a way to get round it.

@ian-ross ian-ross force-pushed the ian-ross:preprocessor-extra-sources branch 6 times, most recently from da4e2c0 to e38cb0c Mar 23, 2015

@ian-ross ian-ross closed this Mar 27, 2015

@ian-ross ian-ross reopened this Mar 27, 2015

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 27, 2015

@23Skidoo OK, I've now done this in a different way. It's kind of ugly, but it doesn't change the user-visible preprocessor API at all, so it ought not to perturb any of the existing packages that use preprocessors in custom builds. Given the constraints on changes to the public Cabal API, this seems like a better way to do things for now.

@ian-ross ian-ross force-pushed the ian-ross:preprocessor-extra-sources branch from e38cb0c to 54ee431 Mar 29, 2015

@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 29, 2015

@23Skidoo Thanks for the detailed review! I've added Haddock comments, added full module names in the error messages, and have changed the way that the extra C sources are searched for, just looking for files matching the relevant pattern under the build directory (which is where these things end up, not in the autogen directory).

@ian-ross ian-ross force-pushed the ian-ross:preprocessor-extra-sources branch 2 times, most recently from 3fc9cdb to a18a288 Mar 29, 2015

Allow preprocessors to specify extra C sources
Add functionality to allow preprocessors like hsc2hs and C2HS to inform
Cabal of extra C sources that they create that need to be compiled and
linked.  Includes hsc2hs-based test case.

@ian-ross ian-ross force-pushed the ian-ross:preprocessor-extra-sources branch from a18a288 to a30b11f Mar 29, 2015

@23Skidoo

This comment has been minimized.

Nice, it also ended up being less code.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 29, 2015

Thanks, everything looks good to me now. Merging.

23Skidoo added a commit that referenced this pull request Mar 29, 2015

Merge pull request #2467 from ian-ross/preprocessor-extra-sources
Allow preprocessors to specify extra C sources

@23Skidoo 23Skidoo merged commit 010093c into haskell:master Mar 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ian-ross

This comment has been minimized.

Copy link
Member

ian-ross commented Mar 29, 2015

Great. That was kind of epic. Thanks for being patient.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 29, 2015

Thanks to you for your contribution!

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