Skip to content
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

code-generators field in test stanza. #7688

Merged
merged 10 commits into from
Feb 25, 2022
Merged

code-generators field in test stanza. #7688

merged 10 commits into from
Feb 25, 2022

Conversation

gbaz
Copy link
Collaborator

@gbaz gbaz commented Sep 27, 2021

This PR extends the test stanza of a cabal file with a code-generators: field containing executables. These executables can be generated as dependencies via the build-tools-depends: field. They are run during the build step after all other preprocessors, and before the main build step, and can be used to autogenerate modules for test suites, e.g. for doctests, or other "discover" style test running.

These executables are expected to follow a very simple api. They are invoked as exe-name TARGETDIR [SOURCEDIRS] -- [GHCOPTIONS] -- where targetdir is a temp directory in the dist-newstyle hierarchy where they are directed to place their generated code. This is followed by a list of source directories containing the source files of all the local lib components which the test stanza depends on. Finally, following a double-dash, are all the options that cabal would pass to ghc for a build.

The expected output of these executables is a newline-separated list of names of all the generated modules. This PR also prevents checking for the existence of the main-is module in the preprocessor step, because that main module itself may be generated by the code generator.

A proof-of-concept of how this works is at https://github.com/gbaz/cabal-doctest-demo -- this repo gives a package that has two different doctest runners built on top of this functionality. One wraps up the doctest program with a wrapper generated from the ghc options (and so which does not rely on a ghc environment file to bring deps into scope, but rather extracts them directly form the build-depends). The other uses the lexer from https://github.com/phadej/cabal-extras/tree/master/cabal-docspec and generates a fully static tasty testsuite from the doctest comments in the source lib.

I think this code is basically correct and clean. If people like it, to finish off this PR it needs a changelog, doc updates, and some proper extension of the cabal test suite.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 27, 2021

This is amazing.

@fgaz
Copy link
Member

fgaz commented Sep 29, 2021

I wonder, could this be generalized to make the existing preprocessor feature extensible?

@jneira
Copy link
Member

jneira commented Sep 29, 2021

I wonder, could this be generalized to make the existing preprocessor feature extensible?

#7394??

@fgaz
Copy link
Member

fgaz commented Sep 29, 2021

I wonder, could this be generalized to make the existing preprocessor feature extensible?

#7394??

that's in cabal-install, while I'm talking about the Cabal/.cabal feature: https://cabal.readthedocs.io/en/latest/cabal-package.html#modules-and-preprocessors

#6726 mentions extensible preprocessors

edit: #6232 too... I wish there was a (properly applied) "preprocessors" tag

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 29, 2021

This is slightly different than standard preprocessors since it generates code from scratch, rather than turning an *.xyz file into a *.hs file. I certainly became familiar with the preprocessor code via working on this, but I think that generalizing preprocessors would have a different api design for how executables are expected to be invoked, etc. So it is related work, but sufficiently different I can't see a single generalization covering both.

@andreasabel andreasabel added the re: doctest Concerning doctest suites label Nov 7, 2021
@Kleidukos
Copy link
Member

@gbaz Hi! I've taken a look at the PR and I was wondering: I see you've used the syntax docspec-static-demo:docspec-static, but it's because you've looked-up the program database right? If I were to put my build-tool on Hackage, cabal would fetch it automatically right?

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 7, 2022

That syntax is existing syntax for how build-tool-depends works -- the first component is the package (pulled from hackage, or whatever other repos are specified, etc) and the second is the component that's built and depended on -- i.e. the particular tool from the package to be brought into the program-database.

The new thing is the code-generators field, which does not use that component syntax, but just looks up the tool by name in the program database.

@Kleidukos
Copy link
Member

@gbaz Thank you very much! Do you still need a hand to finalise this PR? I would also enable the syntax in other places like library components

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 7, 2022

As I discussed on irc I don't think it makes sense in library components because this enables things to generate new modules fed into the build pipeline. For libraries, we want all exposed modules declaratively specified in the file itself.

I'm finishing up the test failures, then will add some tests and docs, but otherwise yeah I'd like some review on it.

@Kleidukos
Copy link
Member

@gbaz Concretely what happens if a module isn't declared in autogen-modules? There is an immediate sanction for the developer right? Is there a difference of feedback / behaviour compared to what people can achieve today with Setup.hs?

From what @swamp-agr showed me in encoding, the process seems to be enforced just like you want for custom setups?

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 7, 2022

I should clarify. The point of this field is that it can generate modules that are not listed in the cabal file, and make use of them.

The use case of autogen-modules is totally different. It allows modules that are listed in the cabal file to not cause failures with sdist when their source does not appear: https://cabal.readthedocs.io/en/3.4/cabal-package.html#pkg-field-autogen-modules

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 7, 2022

What is the point of https://github.com/haskell/cabal/blob/master/Cabal-tests/tests/UnitTests/Distribution/Utils/Structured.hs btw?

Is it just to make us notice when we change the structure hash? I.e. this pr adds stuff to the TestSuite type, which necessarily affects the hash of GenericPackageDescription and LocalBuildInfo. But its ok to do that -- we've updated this file in the past to reflect changes to the hash. So what are we gaining by this?

@Kleidukos
Copy link
Member

Kleidukos commented Jan 8, 2022

I should clarify. The point of this field is that it can generate modules that are not listed in the cabal file, and make use of them.

Ah I see, those two usecases are different. Thank you very much for the explanation. I imagine this justifies the separate build-drivers that I had imagined at the beginning. Then if the user generates modules that are depended on but not listed, Cabal will properly complain. :)

What is the point of https://github.com/haskell/cabal/blob/master/Cabal-tests/tests/UnitTests/Distribution/Utils/Structured.hs btw?

I think @phadej originally introduced it in #6255 / ae33fcf.

@phadej
Copy link
Collaborator

phadej commented Jan 8, 2022

Structured helps with cache expiration. binary can do weird stuff when decoding something. (E.g. if it expects list length and gets some big Int indeed...). The test for GPD hash is there to notice that structure hash changes when it should and don't when it shouldn't.

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 11, 2022

Right. I understand and appreciate structured. Not a big deal but just remain unsure if there's ever a case where these tests will fail and it will mean some action other than "update the tests with the new hash".

@gbaz
Copy link
Collaborator Author

gbaz commented Feb 1, 2022

added changelog, docs, tests. ready for review and merge

@Kleidukos Kleidukos added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Feb 24, 2022
@gbaz gbaz merged commit 9b300f3 into master Feb 25, 2022
Kleidukos pushed a commit to Kleidukos/cabal that referenced this pull request Mar 30, 2022
* wip to add test-code-generators field to test stanzas

* fixups

* change hashes

* regen golden parser test output

* docs and changelog

* test

* Update pr-7688

* tweak test

Co-authored-by: Gershom Bazerman <gershom@arista.com>
andreabedini pushed a commit to andreabedini/cabal that referenced this pull request May 5, 2022
* wip to add test-code-generators field to test stanzas

* fixups

* change hashes

* regen golden parser test output

* docs and changelog

* test

* Update pr-7688

* tweak test

Co-authored-by: Gershom Bazerman <gershom@arista.com>
@@ -268,6 +268,7 @@ data TestSuiteStanza = TestSuiteStanza
, _testStanzaMainIs :: Maybe FilePath
, _testStanzaTestModule :: Maybe ModuleName
, _testStanzaBuildInfo :: BuildInfo
, _testStanzaCodeGenerators :: [String]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String isn't a good name for a type as it does not reflect its intended semantics. It is also badly searchable across the code base.

type CodeGenerator = String
type CodeGenerators = [CodeGenerator]

and then use these type synonyms.
Extra points for a newtype!

@@ -26,7 +26,8 @@ import qualified Distribution.Types.BuildInfo.Lens as L
data TestSuite = TestSuite {
testName :: UnqualComponentName,
testInterface :: TestSuiteInterface,
testBuildInfo :: BuildInfo
testBuildInfo :: BuildInfo,
testCodeGenerators :: [String]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment would be in order explaining this new field, maybe linking to this PR.

In particular I wonder, should this field be non-revisable on Hackage?
(By default, I suppose, "yes, cannot change in a revision". But on the other hand, what is the harm of revising this? It's just for the test suite. Also, version of dependencies can be arbitrary revised, to the point of completely changing functionality... Why not test generaters...?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our principle for allowing revisions has always been "by default, negative, and allow revision only if there's demonstrated need"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the justification.

@andreasabel andreasabel added the re: code-generators Concerning the `code-generators` field/functionality for `testsuite`s label Jun 13, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Jun 20, 2022

@gbaz: what do you plan to do with the extra comments donated to you after the merge?

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 12, 2022

They're useful comments, but I think too small to be actionable after the fact.

@andreasabel
Copy link
Member

They're useful comments, but I think too small to be actionable after the fact.

@gbaz: Not sure I follow this logic.

But I'd anyhow appreciate some discussion/justification about revisability decisions...

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 12, 2022

I left a comment regarding revisability. Otherwise, I just meant, there's plenty else on my plate and I don't imagine I'll find time for stylistic/comment cleanup at this point. Maybe the next time somebody touches the relevant code they could do further work, but for now I'm going to focus elsewhere.

@andreasabel
Copy link
Member

I left a comment regarding revisability.

Thanks!

@sol
Copy link
Member

sol commented Aug 20, 2022

I'm not up to date with all the details of this proposal and, as I'm swamped with other obligations, I'm not planning to get too involved.

hspec-discover does not require this (or any other changes to Cabal)

The current approach is adequate, robust and relies on GHC features alone (read: you can easily use it with GHC, without relying on Cabal).

doctest doesn't need this

By design, doctest should behave as similar to ghci as possible. It also supports the same cli. Hence it makes sense to invoke it the same way as you would invoke ghci. The closest I've come up with is to invoke it with:

cabal repl --with-ghc=doctest

This woks well in most situations and is my current recommendation. The drawback of this approach is that doctest will also be used as a compiler, not only as a ghci substitute. The doctest executable detects when it's asked to compile something and delegates these requests to the ghc on the path. Again, this works well, except for when compiling the ghc-paths package. That's why you want to make sure that all your dependencies are up to date first. Just do a regular build, before invoking doctest:

cabal build && cabal repl --with-ghc=doctest

The full-fledged version, with all the bells and whistles, of this command is:

cabal install doctest --overwrite-policy=always && cabal build && cabal repl --build-depends=QuickCheck --build-depends=template-haskell --with-ghc=doctest

What would indeed help from a Doctest's perspective

If we had a flag --with-ghci or --with-repl which tells Cabal to use a different executable as the REPL command only, that would be neat! This will simplify my currently recommended approach and will make it more robust.

In addition, this caters for all kinds of interesting use cases, like say you could use a drop-in "improved" version of ghci.

What I hope will not happen

  1. The .cabal file format spec is not compact at all. It's big. Adding more "useful features" will only make it bigger. That's why I personally think it makes sense to be conservative here.

  2. I'm also using --with-ghc a lot for similar things, like speeding up test runs with hspec/sensei. Please don't break it**, or if you do so then add a --with-ghci option first.

** read: Don't key the "path to ghc", into the hashes for the nix-style package store!

@Mikolaj
Copy link
Member

Mikolaj commented Aug 20, 2022

Thank you for your remarks. They make a lot of sense.

I'm not up to date with all the details of this proposal

Could you elaborate which proposal you have in mind, e.g., which ticket? This PR is an implementation, not a proposal, and it's finalized and merged and used in (demos of) external tools.

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 20, 2022

@sol what this feature (now merged) allows is writing small drivers so that one need not install doctest or any other executable ahead of time, nor does that executable or cabal need to be invoked with any special incantation.

Instead, with a single addition of a field and a supporting driver, one can simply run cabal test.

An example of such a cabal field and stanza (that works) is at: https://github.com/gbaz/cabal-doctest-demo/blob/main/cabal-doctest-demo.cabal#L36

The supporting driver is at: https://github.com/gbaz/cabal-doctest-demo/blob/main/doctest-codegen-driver/app/Main.hs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cabal: cmd/test historical merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days re: code-generators Concerning the `code-generators` field/functionality for `testsuite`s re: doctest Concerning doctest suites squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants