[WIP] Only buildable tests #7829
Conversation
I plan to add an explicit --only-buildable-tests flag, and then to add a test for it. I will do so by adding a case to this test, which currently tests the two other flags, --enable-tests and --disable-tests, as well as the default behaviour.
boolOpt only supports --enable-tests and --disable-tests, and I want to add --only-buildable-tests, so I need to switch to multiOption. I have not added the third option yet, I want to begin by making sure I haven't broken anything yet.
|
As I had feared, this seemingly no-op change did manage to break several tests. First, |
|
Second, |
|
Edit: all this is wrong and stems from my confusion about the current behaviour, due to me having . . Let me clarify for people viewing this PR, the idea is to make No idea about the failing tests. Feel free to ignore and/or redo them until we get reviewers that know what's going on. Edit2: and perhaps |
I was not planning to change the default, but sure, that sounds like a good default!
Currently, I don't quite understand the semantics of what you are proposing instead. Would there be an |
I'm not blocked, just thinking out loud. I'm working on fixing the tests. |
Oh, sure, 1-1 is actually better. I didn't know this can be that simple. Please ignore my overcomplicated version. BTW, it would be great to add tests of cabal invocations that succeed with |
Isn't that what the test I've already added already does? |
|
I think my confusion stems from this comment: #5079 (comment) I think the third option that @phadej asks for is not just simply the current default behaviour (which is to solve for no tests at all) but it's a behaviour that would be best to have as the default for |
Okay, that really is the crux of the confusion then. I do think that the third option is the same as the current default behavior, and I do think that the current default behavior is to try to find a solution which includes tests, and if that can't be done, to look for a solution which does not include the tests. In fact, that's what my test demonstrates: with no arguments, Meanwhile, |
|
I'm afraid @andreasabel and other people in that ticket report otherwise: #5079 (comment) Perhaps that's because you report what Cabal's Setup interface does, while we mean |
|
Hmm, but your test indeed behaves as you claim, which is completely contrary to my experience. Edit: ok, I take my claims back: I was just runing with "tests: False" in my cabal.project. |
|
I now think, probably all people surprised by the Edit: Would that new flag, when used in Edit: and then I think this new flag is the ideal default in all places (instead of False and True; I have no idea if any tool, e.g., |
Yes. My understanding is that "the new flag" (that is, the current default behavior) causes the solver to pick a plan which may include some tests and exclude others. Which of the included targets are built and/or ran depends on the command:
|
|
This is perfection. I didn't even know this is possible. Edit: I checked and the default Edit: Oh, and the hint that is displayed when Edit2: heh, even the main project files in the cabal repo have |
Are you sure? If people set Thinking about this further, I suspect that even in the case in which people do not explicitly set
I don't follow this bit. Surely cabal should not assume that it is ran from inside a git repo? And why are we grepping for |
And
Oh, that's a shame. If that's true, indeed we can't modify the hint.
I mean, 'we' as developers changing that code, not 'we' as 'cabal when it's running'. I was speaking in the context of |
|
Should we maybe schedule a call? I feel like we're really struggling to communicate in this thread.
Yes.
Depends what you mean by "this problem"! In the case in which a test is not buildable, the current hint does not have the problem that it is misleading, because setting The current hint does have the problem that its suggestion is insufficient to completely solve the problem, as running |
With pleasure. However, this is more a sequential string of my misunderstandings than a block that can be dealt with in one go. And each past case is clarified as far as I'm concerned.
Yes, agreed, this also makes it more likely the user will looks closer at the error message with Edit: and renaming |
|
All right, back to thinking out loud about the failing tests. There seems to be two related problems. The first problem is that the setting The second problem is that the setting |
...and the synonyms second. The first listed option is the one used in the "--help" text.
|
I explored an alley that turned out to be a dead end. It is possible to give multiple synonyms for a command-line flag. I am already using this feature to allow |
That is, the "tests" and "benchmark" settings. I still want to recognize "tests: True", but the setting is no longer a boolean, so "tests: EnableAll" is the new preferred way to specify that we want to enable all the tests.
|
I wrote a custom parser for the |
|
Very strange, it's the parse error on |
|
Ah, I know why the roundtrip error on |
|
And the reason why the parse error on cabal/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Lines 106 to 113 in 6492a46 That comment is simply saying that the parser for What misled me is that the comment is right above the Given this, would it make sense to move that comment somewhere else, perhaps closer to |
|
Definitely, please amend and extend the comments as needed to prevent the misunderstanding it caused you. |
I was confused by the placement of a comment, which seemed to explain why we need 'LegacyProjectConfig' but was in fact explaining how the parsers and pretty-printers for the 'ProjectConfig' configuration file was derived from the descriptions of several command-line options. I was also confused because I didn't realize that there were multiple configuration files, each with their own set of overrides for the fields derived from those command-line options. I am thus attempting to improve the comments by explaining which configuration file each type is for, explaining the trick of deriving fields from options and then overriding some of the fields, and cross-linking between the two places where that trick is used, to remind contributors to edit both places if applicable.
That is, the "tests" and "benchmark" settings. I previously added a custom parser for the 'cabal.project.local' file, this commit also adds an identical parser for the '~/.cabal/config' file.
|
All right, I have tweaked the comments (please let me know if you agree it is an improvement!) and I have added the missing parser. I hope the tests are passing this time! |
the previous commit message said I added a custom parser for both "tests" and "benchmarks", but that was a lie.
|
They do pass, hurray! Now I can move on to actual feature: adding an Now that I am familiar with the way in which flags and options work, I know that the easiest way to implement this would be to parse |
Since this flag selects the default behaviour, we can simply parse it to 'NoFlag', right? Unfortunately, this doesn't quite work: if 'tests: EnableAll' is specified in '~/.cabal/config' or 'cabal.project.config', specifying '--only-buildable-tests' on the command-line will not override the setting.
|
I keep calling them "stanza", but it should be "component type". If a package provides a library and two tests, that's three stanzas (or three components): the library, the first test, and the second test. But it's only two component types: libraries and tests. Each of the |
|
Darn, "OnlyBuildable" might also be an incorrect name! I found this comment which gives a different definition for "buildable" in Cabal: cabal/Cabal/src/Distribution/Types/ComponentRequestedSpec.hs Lines 33 to 37 in 00a2351 Whereas what we mean is whether a build plan which includes that component type can be found. "OnlyPlannable"? "IfPossible"? |
|
This other comment explains that cabal/Cabal/src/Distribution/Simple/Configure.hs Lines 397 to 403 in 00a2351 |
|
Actually, |
|
Thanks for the suggestions! How about |
|
Not bad. What would be the names for the other modes? |
|
|
I see, so |
...to "--enable-tests-when-possible". No version of cabal-install in which the "--only-buildable-tests" name exists, I'm still in the process of implementing that feature and I simply changed my mind as to what it should be called.
configTests and configBenchmarks now have three possible values instead
of the previous two ('True' and 'False'), to reflect the fact that not
specifying a value had a third behaviour:
* DisableAll (same as the previous 'False')
* EnableAll (same as the previous 'True')
* EnableWhenPossible (same as previous 'unspecified')
The goal is to make it possible to force the default behaviour even if
'tests: True' or 'tests: False' is specified in a configuration file,
e.g. by passing --enable-tests-when-possible on the command-line.
Unfortunately, this does not yet work. In this commit, I tried to make
as few changes to the current implementation as possible, by converting
'EnableAll' to 'True' and the other two values to 'False' whenever a
function expects a boolean. As a consequence, passing
--enable-tests-when-possible currently behaves like passing
--disable-tests.
the two branches are "not optional" and "optional", not "not optional" and "not optional".
|
@phadej , do you happen to remember what this uncommented TODO was about? You introduced it in 84d2a5e. cabal/cabal-install/src/Distribution/Client/ProjectPlanning.hs Lines 2602 to 2604 in cc59881 Also, while we're looking at this code, that comment should say "it is an optional stanza, so a testsuite or benchmark", not "it is not an optional stanza, so a testsuite or benchmark", correct? |
Before introducing EnableWhenPossible, the conversion from Flag Bool to Maybe Bool was rather simple: NoFlag -> Nothing Flag True -> Just True Flag False -> Just False After introducing EnableWhenPossible, we now have one extra case: NoFlag -> Nothing Flag EnableAll -> Just True Flag DisableAll -> Just False Flag EnableWhenPossible -> ? We previously mapped it to Just False, but that caused --enable-tests-when-possible to behave like --disable-tests. It needs to be mapped to Nothing instead.
|
Since this mysterious TODO is on a case expression which picks an error message, perhaps it is related to this other TODO, which suggests replacing the Bool we're pattern-matching on with a 3-valued sum-type in order to affect the error message? Note that his 3-valued sum-type is not the same as the cabal/cabal-install/src/Distribution/Client/ProjectPlanning/Types.hs Lines 222 to 234 in cc59881 |
Since there is no response, feel free to amend the comment and TODOs. |
it was not clear what that TODO mark asked us to do. I found a second, clearer TODO which asks us to do something with the line where this first TODO is, so I am guessing that this was the reason for the first TODO, so I am expanding the first TODO to point to the second.
done! They now look like this: cabal/cabal-install/src/Distribution/Client/ProjectPlanning.hs Lines 2603 to 2610 in 8af774b |
mergify
bot
added
the
merge delay passed
ulysses4ever
removed
the
merge delay passed
|
Ah, too bad this PR has stalled. Any chance to pick it up again, @gelisam ? Would be great to have this feature! |
|
Sure, thanks for reminding me about this! |
|
I unfortunately haven't been able to find the time for this, does anybody else want to take over? |
As requested, I am trying to implement an
--only-buildable-testsflag whose behaviour matches the default behaviour when neither--enable-testsnor--disable-testsis specified.This PR is a WIP, as I am still at the beginning of my investigation. So far, I have only added a test which exercises the current behaviour, and I have slightly tweaked how
--enable-testsand--disable-testsare parsed (which slightly affects the--helpoutput). Still, my impression is that cabal is a complex and fragile beast, so I would like to make sure that this small change hasn't already broken something. I am thus following the recommendation and opening a WIP PR to get the tests to run.Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!