-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
[lldb][test] Fix ComputeClangResourceDirectory test when CLANG_RESOURCE_DIR is set #98464
Conversation
…CE_DIR is set This test was looking for an exact string match of the path calculated by starting with lib/liblldb and with bin/lldb. However when CLANG_RESOURCE_DIR is set to something e.g. "../lib/clang/19", the way the initial path is handled is different. Instead of taking the parent of the parent of the binary, that is foo/bin/lldb/ -> foo/, it uses the parent of the binary and appends CLANG_RESOURCE_DIR to that. As CLANG_RESOURCE_DIR is defined as being a path relative to the parent dir's of the clang binary. This means that if you start with foo/lib/lidblldb the resulting path is lib/../lib/clang/19, but if you start with bin/lldb the result is bin/../lib/clang/19. I don't want to change the starting path of DefaultComputeClangResourceDirectory (which is bin/lldb) as I suspect that's chosen instead of liblldb for good reason. So the way to make this test work is to check not for exact path matches but that the "real" (".." resolved) version of the paths are the same. That way foo/bin/../lib and foo/lib/../lib will be the same.
@llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) ChangesThis was found during testing of llvm snapshots for Fedora. This test was looking for an exact string match of the path calculated by starting with lib/liblldb and with bin/lldb. However when CLANG_RESOURCE_DIR is set to something e.g. "../lib/clang/19", the way the initial path is handled is different. Instead of taking the parent of the parent of the binary, that is foo/bin/lldb/ -> foo/, it uses the parent of the binary and appends CLANG_RESOURCE_DIR to that. As CLANG_RESOURCE_DIR is defined as being a path relative to the parent dir's of the clang binary. This means that if you start with foo/lib/lidblldb the resulting path is lib/../lib/clang/19, but if you start with bin/lldb the result is bin/../lib/clang/19. I don't want to change the starting path of DefaultComputeClangResourceDirectory (which is bin/lldb) as I suspect that's chosen instead of liblldb for good reason. So the way to make this test work is to check not for exact path matches but that the "real" (".." resolved) version of the paths are the same. That way foo/bin/../lib and foo/lib/../lib will be the same. Full diff: https://github.com/llvm/llvm-project/pull/98464.diff 1 Files Affected:
diff --git a/lldb/unittests/Expression/ClangParserTest.cpp b/lldb/unittests/Expression/ClangParserTest.cpp
index ed5ee323b7d20..6f682f6c97fdb 100644
--- a/lldb/unittests/Expression/ClangParserTest.cpp
+++ b/lldb/unittests/Expression/ClangParserTest.cpp
@@ -44,7 +44,17 @@ TEST_F(ClangHostTest, ComputeClangResourceDirectory) {
#endif
std::string path_to_clang_dir = clang::driver::Driver::GetResourcesPath(
path_to_liblldb + "liblldb", CLANG_RESOURCE_DIR);
- EXPECT_EQ(ComputeClangResourceDir(path_to_liblldb), path_to_clang_dir);
+ llvm::SmallString<256> path_to_clang_lib_dir_real;
+ llvm::sys::fs::real_path(path_to_clang_dir, path_to_clang_lib_dir_real);
+
+ std::string computed_path = ComputeClangResourceDir(path_to_liblldb);
+ llvm::SmallString<256> computed_path_real;
+ llvm::sys::fs::real_path(computed_path, computed_path_real);
+
+ // When CLANG_RESOURCE_DIR is set, both the functions we use here behave in
+ // such a way that leads to one path being lib/ and the other bin/. Check that
+ // they are equivalent after any ".." have been resolved.
+ EXPECT_EQ(path_to_clang_lib_dir_real, computed_path_real);
// The path doesn't really exist, so setting verify to true should make
// ComputeClangResourceDir not give you path_to_clang_dir.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more robust and I like that it doesn't risk breaking the clang resource directory path. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…CE_DIR is set (llvm#98464) This was found during testing of llvm snapshots for Fedora. This test was looking for an exact string match of the path calculated by starting with lib/liblldb and with bin/lldb. However when CLANG_RESOURCE_DIR is set to something e.g. "../lib/clang/19", the way the initial path is handled is different. Instead of taking the parent of the parent of the binary, that is foo/bin/lldb/ -> foo/, it uses the parent of the binary and appends CLANG_RESOURCE_DIR to that. As CLANG_RESOURCE_DIR is defined as being a path relative to the parent dir's of the clang binary. This means that if you start with foo/lib/lidblldb the resulting path is lib/../lib/clang/19, but if you start with bin/lldb the result is bin/../lib/clang/19. I don't want to change the starting path of DefaultComputeClangResourceDirectory (which is bin/lldb) as I suspect that's chosen instead of liblldb for good reason. So the way to make this test work is to check not for exact path matches but that the "real" (".." resolved) version of the paths are the same. That way foo/bin/../lib and foo/lib/../lib will be the same.
This was found during testing of llvm snapshots for Fedora.
This test was looking for an exact string match of the path calculated by starting with lib/liblldb and with bin/lldb. However when CLANG_RESOURCE_DIR is set to something e.g. "../lib/clang/19", the way the initial path is handled is different.
Instead of taking the parent of the parent of the binary, that is foo/bin/lldb/ -> foo/, it uses the parent of the binary and appends CLANG_RESOURCE_DIR to that. As CLANG_RESOURCE_DIR is defined as being a path relative to the parent dir's of the clang binary.
This means that if you start with foo/lib/lidblldb the resulting path is lib/../lib/clang/19, but if you start with bin/lldb the result is bin/../lib/clang/19.
I don't want to change the starting path of DefaultComputeClangResourceDirectory (which is bin/lldb) as I suspect that's chosen instead of liblldb for good reason.
So the way to make this test work is to check not for exact path matches but that the "real" (".." resolved) version of the paths are the same. That way foo/bin/../lib and foo/lib/../lib will be the same.