Support GN public_deps for the Rust part of a mixed C++-Rust target [chromium/src : main]
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
danakj has uploaded this change for review.
Support GN public_deps for the Rust part of a mixed C++-Rust target
They are supported like other GN variables, by forwarding the
rs_public_deps variable through to the public_deps of the generated
rust_library().
This is important for component builds as a shared library links all
its `deps` and users of the shared library won't have access to them
unless they appear in `public_deps` (or the user adds them to their
own deps).
This is similar to C++ where gn check enforces that headers are not
used unless the target has a dependency on the header's target, either
directly or through public deps. In the future, with GN changes, GN
will enforce and support this correctly for Rust as well.
R=luk...@chromium.org
Bug: 1296156
Change-Id: Ibaaf1f05c8338acac60ab351d077ef27cab5cab7
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,linux-rust-x64-rel
---
M build/rust/mixed_target.gni
M build/rust/rust_static_library.gni
2 files changed, 40 insertions(+), 0 deletions(-)
diff --git a/build/rust/mixed_target.gni b/build/rust/mixed_target.gni
index 8f7ad1c..0666e7a 100644
--- a/build/rust/mixed_target.gni
+++ b/build/rust/mixed_target.gni
@@ -36,6 +36,7 @@
# rs_edition
# rs_configs
# rs_deps
+# rs_public_deps
# rs_test_deps
# rs_skip_unit_tests
# rs_unit_test_target
@@ -79,6 +80,7 @@
"rs_edition",
"rs_configs",
"rs_deps",
+ "rs_public_deps",
"rs_test_deps",
"rs_sources",
"rs_features",
@@ -128,6 +130,9 @@
if (defined(invoker.rs_deps)) {
deps = invoker.rs_deps
}
+ if (defined(invoker.rs_public_deps)) {
+ public_deps = invoker.rs_public_deps
+ }
if (defined(invoker.rs_crate_root)) {
crate_root = invoker.rs_crate_root
}
@@ -164,6 +169,9 @@
if (!defined(deps)) {
deps = []
}
+ if (!defined(public_deps)) {
+ public_deps = []
+ }
# C++ source set depends on the Rust source set
# plus any other things it required C++ targets
diff --git a/build/rust/rust_static_library.gni b/build/rust/rust_static_library.gni
index a2d756f..de31435 100644
--- a/build/rust/rust_static_library.gni
+++ b/build/rust/rust_static_library.gni
@@ -64,6 +64,11 @@
# List of GN targets on which this crate depends. These may be Rust
# or non-Rust targets.
#
+# public_deps (optional)
+# List of GN targets on which this crate depends, and which are exported
+# into the dependency list of any crate that depends on it. Dependency
+# crates that appear in the public API should be included here.
+#
# test_deps (optional)
# List of GN targets on which this crate's tests depend, in addition
# to deps.
To view, visit change 3503825. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
Patch set 3:Commit-Queue +1
1 comment:
Patchset:
I missed some of this change in PS1, so please ignore that. PS3 has the usage of public_deps.
To view, visit change 3503825. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
Patch set 5:Auto-Submit +1
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
Patch set 5:Code-Review +1
3 comments:
Patchset:
LGTM, although you might want to ask Adrian for another look - they are quite a bit more familiar with GN and Rust targets than me. I did leave 2 questions below - I hope they make sense.
File build/rust/mixed_target.gni:
Patch Set #5, Line 12:
mixed_componentNot directly related to this CL / just to double-check: We don't define a `mixed_source_set` right? FWIW, "mixed_source_set" is mentioned [1] in a few comments - maybe worth a separate clean-up CL?
[1] https://source.chromium.org/search?q=mixed_source_set&ss=chromium
AFAICT the new `rs_public_deps` property/variable is not actually used anywhere? Did I miss something?
To view, visit change 3503825. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
2 comments:
File build/rust/mixed_target.gni:
Patch Set #5, Line 12:
mixed_component
Not directly related to this CL / just to double-check: We don't define a `mixed_source_set` right? […]
That's right, it renamed to mixed_static_library. Thanks for pointing out the stragglers.
AFAICT the new `rs_public_deps` property/variable is not actually used anywhere? Did I miss somethi […]
It is converted to public_deps on the line above. Then GN uses it as usual from there.
To view, visit change 3503825. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Patch set 5:Commit-Queue +2
danakj (Gerrit)
Patch set 6:Auto-Submit +1Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: build/rust/rust_target.gni
Insertions: 1, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Support GN public_deps for the Rust part of a mixed C++-Rust target
They are supported like other GN variables, by forwarding the
rs_public_deps variable through to the public_deps of the generated
rust_library().
This is important for component builds as a shared library links all
its `deps` and users of the shared library won't have access to them
unless they appear in `public_deps` (or the user adds them to their
own deps).
This is similar to C++ where gn check enforces that headers are not
used unless the target has a dependency on the header's target, either
directly or through public deps. In the future, with GN changes, GN
will enforce and support this correctly for Rust as well.
R=luk...@chromium.org
Bug: 1296156
Change-Id: Ibaaf1f05c8338acac60ab351d077ef27cab5cab7
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,linux-rust-x64-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3503825
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Auto-Submit: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#978279}
---
M build/rust/mixed_target.gni
M build/rust/rust_static_library.gni
M build/rust/rust_target.gni
3 files changed, 55 insertions(+), 3 deletions(-)
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
1 comment:
File build/rust/mixed_target.gni:
It is converted to public_deps on the line above. Then GN uses it as usual from there.
Right - `rs_public_deps` is *consumed* on the line above. But is there any place where `rs_public_deps` is *provided* as input?
To view, visit change 3503825. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
1 comment:
File build/rust/mixed_target.gni:
Right - `rs_public_deps` is *consumed* on the line above. […]
Right. I found it was missing because I required the use of public_deps in rust_gtest_interop. I had split this out from that change, and that BUILD file isn't making use of this yet. Regardless, a Rust target can be given public_deps, so the inner rust target of a mixed_target should be able to as well.
To view, visit change 3503825. To unsubscribe, or for help writing mail filters, visit settings.