Fix RPATHs for cc toolchain solib when sibling layout is used #17276
Conversation
* Extracts logic to find execroots as a separate method and moves it into the `collectLibrariesToLink()` method from the constructor. * Extracts logic to create rpaths for toolchain libraries as a separate method. * Reorganizes the logic for better readability. This debloats the constructor and makes further changes easier.
|
Still working on the tests for the 2nd commit (65a023b) |
|
@fmeum Could you help review this? |
ShreeM01
added
team-Rules-CPP
src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| public void dynamicLink_siblingLayout_externalBinary_rpath() throws Exception { |
There was a problem hiding this comment.
It would be great to also have an integration test (probably in cc_integration_test.sh).
There was a problem hiding this comment.
Testing toolchain lib rpaths is a bit tricky.
- I need a
cc_toolchain_configwithstatic_link_cpp_runtimesturned on. - I need a
cc_toolchainthat sets thedynamic/static_runtime_libattributes with runtime libs (libc++orlibstdc++). - Since an incorrect rpath does not fail the build, I have to run the binary to verify that it can correctly load the libs.
It seems that 1 and 2 are not supported by the auto-generated toolchain, so that I need to somehow replicate the default toolchain / config and do some modifications, right? Or I can create a custom toolchain - but since there's no hermetic compiler available, I'm not sure how far I could get.
3 means that I can't use a fake runtime lib (can I?), or the binary will fail to load as well. What I can think of is searching for them in the host (again since there is no hermetic compiler available) - I definitely don't want to pull llvm as a test dependency here.
Considering all these requirements, I'm not sure if it is worth the effort. Do you have any pointers for how to implement the test?
There was a problem hiding this comment.
Unless I'm missing something --features=static_link_cpp_runtimes be enough for point 1.
Regarding point 2, would it be useful in the future to have a Starlark label-typed build setting that is able to override the dynamic_runtime_lib and the static_runtime_lib values (i.e. a command line option accepting a file target that overrides those attributes)? If that's actually something that would be widely useful, we could implement that once we starlarkify C++ configurations (or we could add it already with a native LateBoundDefault if it's something that would be really really useful).
If such a flag is not worth the complexity because it would only be useful for this test, I still feel like this should be checked in tested though even if it requires mocking a new toolchain.
There was a problem hiding this comment.
re: --features=static_link_cpp_runtimes, it's not working now. It seems that the static_link_cpp_runtimes feature is not defined in the default unix toolchain config, nor is it added by CppActionConfigs.getLegacyFeatures() because supportsEmbeddedRuntimes is set to false. The relevant commit is ad30d85. Maybe I could add the feature (but disabled) to unix_cc_toolchain_config.bzl, but not sure if it would cause undesirable effects.
re: point 2, one workaround would be duplicating the default cc_toolchain target (e.g. @local_config_cc//:cc-compiler-k8) and setting dynamic|static_runtime_lib on the copy (reusing the default cc_toolchain_config and dependencies). Not sure how good it works cross-platform, though. A config to override them would be welcome, but I'm not sure how wide it can be useful. (Here are a set of more general ideas that I think would be quite useful).
Would you be OK if I add the static_link_cpp_runtimes feature into unix_cc_toolchain_config.bzl (disabled by default)? I think it will be a different PR, though.
There was a problem hiding this comment.
Would you be OK if I add the static_link_cpp_runtimes feature into unix_cc_toolchain_config.bzl (disabled by default)? I think it will be a different PR, though.
Yes, that is a welcome change if that helps getting you there. If it's disabled by default it should be safe.
(#16520 (comment) are a set of more general ideas that I think would be quite useful).
I haven't spent time fully processing your ideas there but please take a look at
https://docs.google.com/document/d/1-etGNsPneQ8W7MBMxtLloEq-Jj9ng1G-Pip-cWtTg_Y/edit?pli=1#heading=h.5mcn15i0e1ch and tell me if that kind of solves the same problem you are trying to solve with those ideas.
There was a problem hiding this comment.
#17391 to add the feature.
Thanks for sharing the proposal! That doc fully covers points 2 and 3 in my comment regarding the toolchain, and mostly covers point 5 (although is a P2). It could potentially also cover point 1 (I left a comment on the proposal). The author mentioned in the document history that there will be another proposal for something similar to point 4 (I hope) - for me point 4 is a killer feature that I'm implementing with some custom rules but still rough due to limitations in the current cc_toolchain / cc_toolchain_config rules.
re: ideas about the build rules, they apparently belong to a separate proposal.
So yeah this proposal definitely overlaps with many of my ideas. Glad and can't wait to see the improvements there!
There was a problem hiding this comment.
I don't think any of this unblocks you immediately though. Since I don't think we have a way of unblocking you right now and the only option is to add a lot of code for mocking the toolchain which may be discarded later once the better mechanisms are available, I'd be fine with a comment specifying what will be needed before we can have a test and that it will get written as soon as possible. Meanwhile we can check this in without a test.
There was a problem hiding this comment.
Let me know if I missed something and you can indeed add a test easily already or alternatively let me know if this PR doesn't block you and it can wait for quite a while till the cc_toolchain design is implemented.
If it needs to go in now already to unblock you, please just add the comment and I will approve.
There was a problem hiding this comment.
Added a TODO item in the test class.
I don't think there is an easy way to do that yet. This PR will block me soon so yeah let's get it merged.
Thank you all for the review @oquenchil @fmeum!
src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
Outdated
Show resolved
Hide resolved
When sibling repository layout is used, the toolchain solib directory is not co-located with the main solib (solib_<cpu>). Therefore, the RPATHs for toolchain solib need to be computed separately, in a similar manner as how they are computed for the main solib. This change uses a private implementation of `getRelative()` function, rather than the `PathFragment.relativeTo()` method. The reason is that `PathFragment.relativeTo()` requires the base path to be a prefix which is not true in the cases here. Fixes #16956
oquenchil
added
awaiting-PR-merge
|
The failing tests seem to be flakes. |
This should fix issue #16956 Includes the following changes: - Refactor logics to find runtime library search directories - Correctly compute RPATHs for toolchain solib. Closes #17276. PiperOrigin-RevId: 509815187 Change-Id: I7f2a2412aea6430fa712dd2836e18f9536757753 (cherry picked from commit 773d232) Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
This should fix issue #16956
Includes the following changes: