[SYCL][E2E] Check for run-time features when in build-only #17988
Conversation
|
CI seems failing, ping me when PR is updated with the fix :) |
@sarnex, should be good to check now. |
| @@ -36,6 +36,12 @@ class E2EExpr(BooleanExpression): | |||
| "vulkan", | |||
| "hip_options", | |||
| "cuda_options", | |||
| "host=None", | |||
There was a problem hiding this comment.
Do you mind explaining why we need to error if an unknown feature is true at build time? If there's an feature not in this list and it evaluates as true at build time doesn't that tell us it's a build-time feature and we don't need the list?
There was a problem hiding this comment.
In build-only we ignore the run-time features because we expect them to always evaluate to false. So when a feature is not marked as build specific and it evaluates to true it means one of two things: It is a run-time feature which is incorrectly evaluating to true in build-only, or it is a build feature that was not added to the build features list, and thus we are incorrectly ignoring it. The latter is the more common case which I'm trying to make more visible. Since prior to these changes we would catch these issues only if the feature evaluated to true on the system, and was used in a REQUIRES/UNSUPPORTED.
The reason why we need the list, and cant just rely on the features that end up evaluating to true on build-only is because some of these features may evaluate to false on other systems/configurations, but we know that they should not be ignored. i.e., if we are missing the vulkan library we will fail building tests that require vulkan so this feature should not be ignored even if it evaluates to false.
There was a problem hiding this comment.
got it, thanks for the clear explanation
There was a problem hiding this comment.
it means one of two things
Or, third option, this was mis-spelled and there is no such feature. Although maybe we catch that somewhere else now (there was a PR but I'm not sure if it was merged, @AlexeySachkov , @KornevNikita , @dm-vodopyanov might know?..)
There was a problem hiding this comment.
Unfortunately this PR drowned in discussion (if you mean this one) ((maybe one day it will be finished)).
There was a problem hiding this comment.
you linked to this pr, is that intentional?
There was a problem hiding this comment.
lol of course not, I mean this one - #16019
Currently when we check
REQUIRES/UNSUPPORTEDstatements inbuild-onlymode we throw an error if any feature that is not inbuild_specific_featuresevaluates to true. Since this is error will only trigger when a test queries for one of these features, it can be easy to miss adding new build-specific features to this list (see #17985, and #17363).This pr changes this check to be done for all features when in
build-only, not just those queried by a test. If any available feature does not appear inbuild_specific_featuresthen an error is thrown.