Only use cpphs for .cpphs files

[vmchale]

This addresses #4278. I think mboes is correct and it should simply fail to process .cpphs files if cpphs is not available, especially since new-build and build-tool-depends: cpphs:cpphs

• 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.

Tested by building the exe and running it locally.

[Mikolaj]

Would there be a way to cheaply test this in our test suite? Is it blocking you and would a backport to 3.8 potentially help?

[robx]

What's this export for? Is the function now unused and could be deleted instead?

[vmchale]

I figured users might want it - seemed safest to allow it in custom Setup.hs if they really want.

[robx]

Hmm I'd tend to not extend the API unless there's a clear need (and we add some test coverage). But it's not for me to decide.

[vmchale]

@Mikolaj I'd say a backpack to 3.8 would be useful, yes! GHC 9.4.1 has a bug in its preprocessor.

[Mikolaj]

@Mikolaj I'd say a backpack to 3.8 would be useful, yes! GHC 9.4.1 has a bug in its preprocessor.

3.8 is the Cabal version shipped with 9.4.1. Backport to 3.6 is not likely. An option is include this PR only in 3.10 that is not yet even planned, but I gather we'd want to avoid that.

[robx]

In principle the change looks fine to me, but I don't understand the topic well enough to give a full review -- this change does seem to have the potential break things.

It's a bit unfortunate that there doesn't seem to be any pre-existing test coverage for .cpphs files, that would make things a bit easier to evaluate.

[Mikolaj]

Let me rebase to include a workaround to our CI instability and fix the tests.

[Mikolaj]

@mergify rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[ulysses4ever]

Wow: it just said that it was me who pushed the rebased committs?..

[andreasabel]

A test, please.
By principle, every new feature and every bug fix needs a test to secure it from rotting.

[Mikolaj]

I think, once we have a test, it's good to go.

[Mikolaj]

BTW, we are ready to assist with modifying an existing test or writing a new one.

[ulysses4ever]

Has anyone thought about what would a test amount to? For one (it seems to me), it'd require cpphs installed in the test environment. Does anyone know of a way to get a Hackage-hosted tool installed in the test environment for one test only? From what I know, I think it's only possible to install stuff globally for all tests. Do we want that?

[Mikolaj]

I don't know. Perhaps the new feature for specifying an exe build dependency would be enough (forgot how it's called). I mean, perhaps we could add cpphs as such dependency to the .cabal file of the single test in question.?

[ulysses4ever]

@Mikolaj my impression (correct me if I'm wrong) was that those cabal files don't have access to Hackage; see FAQ for the testsuite, Q#3.

[Mikolaj]

Right, I now remember a discussion when somebody wanted the access to check messages from cabal update. How big is cpphs? Would it be cached?

[ulysses4ever]

It's very small, both in terms of dependency footprint and in terms of code size. On my laptop it took several seconds to build. And yes, it'd probably be in the cache all the time.

It's not the build time that I'm concerned about, but just expanding the CI script, and the precedent of doing that for a single test. But maybe it's not worth worrying.

[Mikolaj]

Let's expand this one last time. :)

[AntC2]

(Apologies if I'm talking out of turn here -- just ignore me. By some cosmic coincidence -- spotted by @vmchale -- a few days ago I found cpphs answered a specific need, and for which off-the-shelf cpp wasn't helping.)

Has anyone thought about what would a test amount to?

cpphs is more friendly for Haskell code than cpp -- that is, if you tell it the right way to do things. I wrote up some notes for the cpphs options that might be helpful for a tester.

• A good test would be to expand a multi-line macro into a multi-line Haskell decl with --layout, making sure it indents properly.
• Another good test would be expansions with -XMagicHash; and possibly defining an operator (##). Hashes have special significance within cpp, which make them difficult to use in your Haskell. If you want a test to break something, try those same tests with --hashes set.
• By default, cpphs produces a header line in the output starting #line .... I guess GHC won't know what to do with that(?) Option --noline suppresses it.
• As my note says, cpphs's style for calling macros is myMacro(someArg) -- needs the parens round the arg, not Haskell style myMacro someArg -- whitespace between them. What's bad is that if you use Haskell style, cpphs doesn't shout at you, it'll pass on myMacro someArg into the generated source, where presumably myMacro is not defined. I would include a expected fail test for this, to make sure GHC handles it gracefully.

[Mikolaj]

@AntC2: thank you for the helpful ideas.

@vmchale: could we assist you in any way?

[Kleidukos]

Marking this PR as draft 🙂