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

Check that a test stanza is disabled before generating the rules #6134

Closed
wants to merge 3 commits into from

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Sep 7, 2022

Unlike the executables stanza, Dune attempted to create some rules for a tests even if it was disabled. This adds a similar check as the one for executables to abort rule generation if the stanza is disabled.

This should fix #6132

@rgrinberg rgrinberg added this to the 3.5.0 milestone Sep 8, 2022
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Please describe the test and add a CHANGES entry.

voodoos added a commit to voodoos/dune that referenced this pull request Sep 8, 2022
When a test is disabled dune shouldn't check the availability of its
dependencies.

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
This is what is done for executables: no rule is generated if the enabled_if field evaluates to false.

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Copy link
Collaborator Author

voodoos commented Sep 8, 2022

Thanks for the review Rudi !
I added a comment and a changelog entry.

@emillon
Copy link
Collaborator

emillon commented Sep 9, 2022

It's a dupe of #5529 isn't it? In which case the same review comment applies - we want to preserve the behavior and somehow version it or add a knob to (test) to say what (enabled_if) applies to.

@voodoos
Copy link
Collaborator Author

voodoos commented Sep 13, 2022

It is in fact a duplicate... with exactly the same changes, so there is no reason to keep this PR open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support recursive enable_if in tests
3 participants