[tests] Conditionally skip valgrind xfails using llvm::sys #281
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
|
This doesn't seem to work yet, I think we should hold on merging it |
|
Yeah, just noticed that... |
|
strange, this was building locally with no problems |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #281 +/- ##
===========================================
+ Coverage 54.31% 72.20% +17.89%
===========================================
Files 8 8
Lines 2957 2961 +4
===========================================
+ Hits 1606 2138 +532
+ Misses 1351 823 -528 |
|
clang-tidy review says "All clean, LGTM! 👍" |
unittests/CppInterOp/Utils.h
Outdated
| #ifndef __APPLE__ | ||
| #include <valgrind/valgrind.h> | ||
| #endif //__APPLE__ | ||
|
|
||
| #ifdef __APPLE__ | ||
| #define RUNNING_ON_VALGRIND 0 | ||
| #endif //__APPLE__ | ||
|
|
There was a problem hiding this comment.
| #ifndef __APPLE__ | |
| #include <valgrind/valgrind.h> | |
| #endif //__APPLE__ | |
| #ifdef __APPLE__ | |
| #define RUNNING_ON_VALGRIND 0 | |
| #endif //__APPLE__ | |
| #include "llvm/Support/Valgrind.h" |
There was a problem hiding this comment.
this doesn't work as the preprocessor macro isn't visible anymore. Maybe switching to the original usage of llvm::sys::RunningOnValgrind will work, but it uses the same directive to determine if we are running on valgrind.. This way we avoid the extra function calls
There was a problem hiding this comment.
Which preprocessor macro? How does this work for llvm then?
There was a problem hiding this comment.
it's something like:
#if HAVE_VALGRIND_VALGRIND_H
#include <valgrind/valgrind.h>
bool llvm::sys::RunningOnValgrind() {
return RUNNING_ON_VALGRIND;
}
#else // !HAVE_VALGRIND_VALGRIND_H
bool llvm::sys::RunningOnValgrind() {
return false;
}Let me try again with the previous usage to see if it works now, with the apt install
update : it doesn't seem to work..I think we can go ahead with the current working implementation
There was a problem hiding this comment.
Unfortunately I do not think so. Do we need to rebuild the cached llvm or something?
There was a problem hiding this comment.
@vgvassilev Ah I think I got it: we aren't setting HAVE_VALGRIND_VALGRIND_H in the llvm config, but it is something that should be automatically checked and set by llvm's cmake:
check_include_file(valgrind/valgrind.h HAVE_VALGRIND_VALGRIND_H)
It's used in llvm/Support/Valgrind to conditionally include the valgrind/valgrind.h headers.
But doesn't happen because we use a cached llvm that was built without valgrind at the time of build.
I think we need to rebuild the cache on all the platforms, after installing valgrind(on ubuntu) and it should work.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
No description provided.