Skip to content

[SYCL][NFC] Re-enable, fix and upgrade headers testing #16117

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

Conversation

AlexeySachkov
Copy link
Contributor

We have a suite to check that every header contains enough #include directives and forward declarations so that it can be included standalone and used. However, that suite got accidentally disabled some time ago in #14879

This PR re-enabled the suite and fixed all issues it detected.

Besides that, the suite is upgraded:

  • it now scans source directory, meaning that it won't complain about headers which were removed from the codebase, but left in build folder
  • it is now possible to have add a custom test instead of auto-generated one: this is useful when you want to trigger certain template instantiations to make sure that all code paths are covered

@aelovikov-intel
Copy link
Contributor

it now scans source directory, meaning that it won't complain about headers which were removed from the codebase, but left in build folder

Not sure if that's a bug or a feature... I found it somewhat useful to scan build directory.

@AlexeySachkov
Copy link
Contributor Author

it now scans source directory, meaning that it won't complain about headers which were removed from the codebase, but left in build folder

Not sure if that's a bug or a feature... I found it somewhat useful to scan build directory.

I took a brief look at how many headers we generate and I haven't found anything important - i.e. those version.hpp headers we can probably ignore for now for this test. But if you want, I can try and think about how to include them as well.

Me and other folks encountered a situation when you switch between branches and build directory contains headers which had been previously removed, but still stay there until you do clean build. I.e. I was asked a couple of times something like "why do I see those weird errors at sycl branch without local changes?"

I've also just realized that I forgot to update the documentation we have for the sub-suite.

Comment on lines +34 to +36
# To respect SYCL_LIB_DUMPS_ONLY mode
if ".cpp" not in localConfig.suffixes:
return
Copy link
Contributor

@aelovikov-intel aelovikov-intel Nov 19, 2024

Choose a reason for hiding this comment

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

This variable makes little sense to me... What's wrong with LIT_FILTER=.dump ? I'd rather make sure that LIT_FILTER is handled correctly here and drop our hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable makes little sense to me... What's wrong with LIT_FILTER=.dump ? I'd rather make sure that LIT_FILTER is handled correctly here and drop our hack.

I think that LIT_FILTER=.dump should still work, but most likely the author of the following code was not aware of that feature:

dump_only_tests = bool(lit_config.params.get("SYCL_LIB_DUMPS_ONLY", False))
if dump_only_tests:
config.suffixes = [".dump"] # Only run dump testing

@aelovikov-intel
Copy link
Contributor

Me and other folks encountered a situation when you switch between branches and build directory contains headers which had been previously removed

Yes, and when these self-contained tests fail it's a remainder to remove these stale headers. It's the right thing to do anyway, because that will catch errors if removed header is still referenced and is taken from this stale build directory.

@AlexeySachkov
Copy link
Contributor Author

Me and other folks encountered a situation when you switch between branches and build directory contains headers which had been previously removed

Yes, and when these self-contained tests fail it's a remainder to remove these stale headers. It's the right thing to do anyway, because that will catch errors if removed header is still referenced and is taken from this stale build directory.

The thing is, even the old version won't immediately fail once we remove a header - it will only fail if that header breaks. For example, if headers that it includes stopped to provide some definitions. Therefore, it shouldn't be considered to be a reliable mechanism for that.

If that is something we would like to support, I can look into it, i.e. the script could scan build directory first and then check if that still exists in source directory and if we have a special test for it.

An alternative here is to tweak our CMakeLitst.txt so that it wipes out build/include on every source/include change - but that looks rather radical, because it will introduce some unnecessary overhead in absolute most cases.

@AlexeySachkov AlexeySachkov merged commit 8b719e9 into intel:sycl Nov 21, 2024
12 checks passed
@AlexeySachkov
Copy link
Contributor Author

@intel/llvm-gatekeepers, I've done local merge + check-sycl and results were good. However, there could still be some other in-flight PRs which modify SYCL headers, but don't have those tests re-enabled and therefore they may result in some unexpected post-commit (and later, pre-commit) failures. Depending on the failure it may not be absolutely trivial to fix, but those tests have an XFAIL mechanism as well, even though its a bit non-standard due to the nature of those tests - I would suggest to use it as a temporary quick measure if we encounter any problems with those tests.

@AlexeySachkov AlexeySachkov deleted the private/asachkov/upgrade-self-contained-headers-testing branch November 21, 2024 08:59
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.

2 participants