feature: introduce link_deps attribute for native library linkage#4024
feature: introduce link_deps attribute for native library linkage#4024Suyashagarw wants to merge 1 commit into
Conversation
|
|
||
| These are typically `cc_library` targets. | ||
| """), | ||
| providers = [[CcInfo], [rust_common.crate_info]], |
There was a problem hiding this comment.
Why the [rust_common.crate_info] part here? Naively, while we should allow for rust_library dependencies here, they by themselves already provide CcInfo, so they should satisfy the [CcInfo] spec already?
| deps = attr.label_list( | ||
| doc = dedent("""\ | ||
| List of other libraries to be linked to this library target. | ||
| List of other Rust libraries to be linked to this library target. |
There was a problem hiding this comment.
We were chatting about this internally (don't add link_deps to rust_proc_macro-s). I think externally, we should still not add link_deps to proc_macros, but crucially here we should preserve the pre-existing API: c++-only deps of proc-macros are still OK. So we should change this back to not be specific to Rust libraries.
|
|
||
| link_deps_test = analysistest.make(_link_deps_test_impl) | ||
|
|
||
| def _link_deps_failure_test_impl(ctx): |
There was a problem hiding this comment.
hey, we don't seem to actually be using this (instantiating) in tests?
| target = analysistest.target_under_test(env) | ||
|
|
||
| # Verify that CcInfo (symbols) is present | ||
| asserts.true(env, CcInfo in target, "Target should provide CcInfo from link_deps") |
There was a problem hiding this comment.
This doesn't seem to test much with link_deps -- it asserts that the (client) target_under_test (in the BUILD file main_lib) produces a CcInfo. Instead, you probably want to have the test code examine the client providers of the target under test, locate the information about the link_deps dependency and examine that. Look at the DepsInfo provider, and its direct_crates and transitive_noncrates fields.
| srcs = ["lib.rs"], | ||
| link_deps = [":leaf_lib"], | ||
| tags = ["manual"], | ||
| ) |
There was a problem hiding this comment.
This analysistest suite is nice.
Could you also add a minimal non-analysistest build-test example: something like a cc_library that defines an extern "C" function and a rust_binary that refers to it via link_deps and uses the symbol (so the symbol must be available at link-time, but only there), and a build test that ensures that the rust_binary links successfully.
This change adds a dedicated link_deps attribute to Rust rules, allowing native symbols to be linked without exposing them as Rust crates. Key changes: - Added link_deps to _common_attrs for CcInfo/CrateInfo targets. - Filtered link_deps out of rust_proc_macro to avoid transition complexity. - Implemented transform_link_deps to extract linkage context while hiding crate metadata. - Added a proactive warning for native libraries incorrectly listed in deps. - Added unit tests in test/unit/link_deps to verify correct linkage.
2 participants
This change adds a dedicated link_deps attribute to Rust rules, allowing native symbols to be linked without exposing them as Rust crates.
Closes #4023
Key changes:
This provides a cleaner separation of concerns between Rust modules and native code linkage while maintaining full backward compatibility for existing targets.