-
Notifications
You must be signed in to change notification settings - Fork 15k
[llvm][test][NFCI] Use absolute build path instead of a relative path #159126
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
|
I don't think it's a good idea for me to review this. I've added some other reviewers, who reviewed the original PR that introduced the PR. Also pinging @Tcc100, since they were the one to originally introduce the test. |
|
I suppose this is only an issue because the plugin library is built into unittests, and not lib? The other sample plugin binaries just go to the ordinary lib directory |
Scoping to the root build directory instead of using the path directly is awkward and the only such occurrence in the test suite. It's also prone to breakage for downstreams that change the library path. But it's not even necessary: during build we have the appropriate RPATHs set so we can just depend on the dynamic loader to find it. This extra logic is probably just copy-paste from PluginsTest.cpp. Additionally: * Removed TargetParser as a dependency because it doesn't seem to actually be used. * Moved `add_dependencies()` to `DEPENDS` to better match the rest of LLVM.
5c3184d to
2b5b107
Compare
|
@arsenm Correct. The other sample plugins don't have any But I had another look now and managed to simplify this further. We can actually put the plugin into lib and just rely on the RPATH for it to be found. It's marked |
|
This change introduced test failures for me: My educated guess is that |
|
This path still seems wrong: I'm seeing: |
|
Yeah, I can confirm it passes now. |
As proven by llvm#159126 it's unnecessary to explicitly construct the path for loading the test plugins. On all supported platforms the RPATH is set appropriately for them to be found by the dynamic loader itself. Thus we can just rid ourselves of the extra code. This also cleans up some of the CMake code and C++ includes slightly. Surprisingly `intrinsics_gen` isn't necessary for any of them as none of them actually do much. See: llvm#159126
As proven by llvm#159126 it's unnecessary to explicitly construct the path for loading the test plugins. On all supported platforms the RPATH is set appropriately for them to be found by the dynamic loader itself. Thus we can just rid ourselves of the extra code. This also cleans up some of the CMake code and C++ includes slightly. Surprisingly `intrinsics_gen` isn't necessary for any of them as none of them actually do much. See: llvm#159126
As proven by llvm#159126 it's unnecessary to explicitly construct the path for loading the test plugins. On all supported platforms the rpath is set appropriately for them to be found by the dynamic loader itself. Thus we can just rid ourselves of the extra code. The only exception is `Support/DynamicLibrary` as it also runs on Windows, which lacks the concept of a rpath. This also cleans up some of the CMake code and C++ includes slightly. Surprisingly `intrinsics_gen` isn't necessary for any of them as none of them actually do much. See: llvm#159126
Scoping to the root build directory instead of using the path directly is awkward and the only such occurrence in the test suite. Pass the appropriate substitution instead for this.
Background
In my downstream
%llvmshlibdiris much more nested so the single../is insufficient.But we can use a much more direct substitution rather than a round-about way, which is both cleaner and avoids me needing to patch this test.