-
Notifications
You must be signed in to change notification settings - Fork 696
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
Introduce new ComponentEnabledSpec, removing testEnabled/benchmarkEna… #3542
Conversation
enabledTests = filter testEnabled . testSuites | ||
|
||
-- | Perform an action on each buildable 'TestSuite' in a package. | ||
withTest :: PackageDescription -> (TestSuite -> IO ()) -> IO () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are this removed because we favour withTestLBI
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Unfortunately a backwards compatible interface is not possible because you really do need the CLBI to determine what is enabled.
I guess I need to push the change through cabal-install too. |
Hmm, I just realized that if I remove 'withTest' and 'withBenchmark' then I really ought to remove 'withLib' and 'withExe'. Maybe it's better to keep them and note that there's been a change in behavior. |
@ezyang you could |
I decided to put them back in with copious hyperlinks. |
OK so I came up with a way to do Notes that get rendered by Haddock (since the info is relevant to clients). Along the way I fixed a lot of Haddock documentation. Take a look. |
I guess this PR is blocked on #3545. |
Unblocked! |
Calling attention to the And the rest:
After fixing BC, my plan is to push this and the hack for #3545; the yak for that ticket can be shaved later. |
See the comment in the code. I don't like it but it's BC! Signed-off-by: Edward Z. Yang <[email protected]>
…bled. As per an existing TODO in the code, the use of testEnabled/benchmarkEnabled to indicate if a component was enabled/disabled by the user command line was an egregious violation of abstraction. This commit removes these two fields, instead passing along the necessary enabling information with ComponentEnabledSpec instead. As there were not many uses of testEnabled/benchmarkEnabled, this was not too difficult to do. Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
-- | Perform an action on each buildable 'Benchmark' in a package. | ||
-- You probably want 'withBenchLBI' if you have a 'LocalBuildInfo', see the note in | ||
-- "Distribution.Simple.LocalBuildInfo#buildable_vs_enabled_components" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This note lives in Cabal/Distribution/Types/ComponentEnabledSpec.hs
now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 463c9a8.
…bled.
As per an existing TODO in the code, the use of
testEnabled/benchmarkEnabled to indicate if a component
was enabled/disabled by the user command line was an
egregious violation of abstraction. This commit removes
these two fields, instead passing along the necessary
enabling information with ComponentEnabledSpec instead.
As there were not many uses of testEnabled/benchmarkEnabled,
this was not too difficult to do.
Signed-off-by: Edward Z. Yang [email protected]