Skip to content

[SYCL][E2E] Check for run-time features when in build-only #17988

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: sycl
Choose a base branch
from
Open

[SYCL][E2E] Check for run-time features when in build-only #17988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2e28cb
[SYCL][E2E] Check for run-time features when in build-only
ayylol Apr 11, 2025
cf608a8
Add non-root-user to build features list
ayylol Apr 11, 2025
def789c
Merge branch 'sycl' into build-feature-check
ayylol Apr 14, 2025
186727d
Remove whitespace changes
ayylol Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions sycl/test-e2e/E2EExpr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ class E2EExpr(BooleanExpression):
"vulkan",
"hip_options",
"cuda_options",
"host=None",
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks for the clear explanation

Copy link
Contributor

Choose a reason for hiding this comment

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

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?..)

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this PR drowned in discussion (if you mean this one) ((maybe one day it will be finished)).

Copy link
Contributor

Choose a reason for hiding this comment

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

you linked to this pr, is that intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

lol of course not, I mean this one - #16019

"target=None",
"shell",
"non-root-user",
"llvm-spirv",
"llvm-link",
"true",
"false",
}
Expand Down Expand Up @@ -65,14 +71,10 @@ def evaluate(string, variables, build_only_mode, final_unknown_value=True):
def parseMATCH(self):
token = self.token
BooleanExpression.parseMATCH(self)
if token not in self.build_specific_features and self.build_only_mode:
if token not in E2EExpr.build_specific_features and self.build_only_mode:
self.unknown = True
else:
self.unknown = False
if self.value and self.unknown:
raise ValueError(
'Runtime feature "' + token + '" evaluated to True in build-only'
)

def parseAND(self):
self.parseNOT()
Expand Down Expand Up @@ -112,6 +114,18 @@ def parseAll(self):
self.expect(BooleanExpression.END)
return self.final_unknown_value if self.unknown else self.value

@staticmethod
def check_build_features(variables):
rt_features = [x for x in variables if x not in E2EExpr.build_specific_features]
if rt_features:
raise ValueError(
"Runtime features: "
+ str(rt_features)
+ " evaluated to True in build-only\n"
+ "If this is a new build specific feature append it to:"
+ "`build_specific_features` in `sycl/test-e2e/E2EExpr.py`"
)


import unittest

Expand Down Expand Up @@ -164,11 +178,12 @@ def test_basic(self):
self.assertFalse(
UnsupportedBuildEval("linux && (vulkan && rt_feature)", {"linux"})
)
# runtime feature is present in build-only
# Check that no runtime features are present in build-only
with self.assertRaises(ValueError):
RequiresBuildEval("rt_feature", {"rt_feature"})
E2EExpr.check_build_features({"rt-feature"})
with self.assertRaises(ValueError):
UnsupportedBuildEval("rt_feature", {"rt_feature"})
E2EExpr.check_build_features({"build-only", "rt-feature"})
E2EExpr.check_build_features({"build-mode"})


if __name__ == "__main__":
Expand Down
6 changes: 6 additions & 0 deletions sycl/test-e2e/lit.cfg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,12 @@ def remove_level_zero_suffix(devices):
clangxx += config.cxx_flags
config.substitutions.append(("%clangxx", clangxx))

# Check that no runtime features are available when in build-only
from E2EExpr import E2EExpr

if config.test_mode == "build-only":
E2EExpr.check_build_features(config.available_features)

if lit_config.params.get("print_features", False):
lit_config.note(
"Global features: {}".format(" ".join(sorted(config.available_features)))
Expand Down