Could we stopping putting every native dependency in the library search path?

[danakj]

Hello,

In 86d2534a97d1f8fc, GN began putting the path to every native dependency of a rust target into the library search path. It does this by getting the directory of the native lib and adding it as `-Lnative=<path>`.

This is causing havoc for us in Chrome as Rust targets start to depend on more C++ code, as it creates the opportunity for collisions between `libs` and any C/C++ target with the same name that ends up in the dependency tree of a Rust-linked target (such as a Rust exe).

A concrete example. There's some weird stuff involved here with empty lib files, but please ignore that as it's not fundamentally part of the issue. Any name collision, which is easy since GN targets have short names and rely on the target_out_dir to separate them, will create this problem for Rust.

1. Our Rust stdlib targets need to depend on PartitionAlloc (for allocation shimming).

2. PartitionAlloc has `public_configs` which adds `libs = [ "sync" ]` on Fuchsia, as well as a dependency on the Fuchsia SDK.

3. Our allocation shim `source_set` must put PartitionAlloc a `public_deps` in order to pass along the `public_configs` and have the link include `-lsync`.

4. The sync library part of the Fuchsia SDK, and the GN rules for the SDK add the directory to `lib_dirs` as `third_party/fuchsia-sdk/sdk/obj/x64-api-18/lib`, which puts it on the command line in `-Lnative=../../third_party/fuchsia-sdk/sdk/obj/x64-api-18/lib` and this makes the `-lsync` (from 3) work, or it should.

5. Because of the change in 86d2534a97d1f8fc, every target_out_dir that holds a dependency of the building Rust target is also passed on the command line as `-Lnative=<path>`.

6. There is also a native target named sync in our output directory. In this case it happens to be an empty lib mirroring the fuchsia SDK as `obj/third_party/fuchsia-sdk/sdk/pkg/sync/libsync.a` however any C/C++ GN target that the Rust target depends on with the same name would collide. This archive is on the command line, as it's linked directly into the Rust executable.

7. But because (5) eagerly adds `-Lnative=obj/third_party/fuchsia-sdk/sdk/pkg/sync` to the command line, the linker ends up finding the wrong `libsync.a` and nothing links due to missing symbols.

What are the -Lnative directives for native dependencies trying to achieve?

The goal of this change was to support `#[link]` directives. A rust target may choose to link libraries, which is done by their name only. Any library it names in `#[link]` needs to be in the library search path.

In most cases (well, in all cases in Chrome), when there is a `#[link]` directive, we explicitly add the library it's referring to as a dependency along with a `config` that adds the directory to the `lib_dirs` if needed.

What problems do the -Lnative directives for native dependencies cause?

The vast majority of C/C++ targets in Chrome's codebase (and probably fuchsia?) are not referred to by`#[link]`. By eagerly supporting this for every dependency, we pollute the library search path with the `target_out_dir` for many many C/C++ targets. If any one of them happens to have a conflicting GN target name with some library in a `#[link]` directive, we can end up linking the wrong thing.

What (I think) would be better?

I would like to remove these `-Lnative` directives for native dependencies. When I do so, Chrome continues to build without any issue, and it correctly links with `-lsync` the libsync.a from the fuchsia SDK, as specified by `lib_dirs`.

If a Rust target has a C/C++ dependency and is using `#[link]` to refer to it, we can place the target_out_dir into `lib_dirs` to find it.

Does Fuchsia rely on these `-Lnative` directives?

When I apply the following patch to GN, Chromium continues to build (and can successfully build with the PartitionAlloc dependency added that I need to add per above):

diff --git a/src/gn/ninja_rust_binary_target_writer.cc b/src/gn/ninja_rust_binary_target_writer.cc
index a400cf7d..3fab1217 100644
--- a/src/gn/ninja_rust_binary_target_writer.cc
+++ b/src/gn/ninja_rust_binary_target_writer.cc
@@ -348,20 +348,6 @@ void NinjaRustBinaryTargetWriter::WriteExternsAndDeps(
     path_output_.WriteDir(out_, dir, PathOutput::DIR_NO_LAST_SLASH);
   }
 
-  UniqueVector<SourceDir> nonrustdep_dirs;
-
-  // Non-Rust native dependencies. A dependency from Rust implies the ability
-  // to specify it in #[link], and GN will ensure that rustc can find it by
-  // adding it to the native library search paths.
-  for (const auto& nonrustdep : nonrustdeps) {
-    nonrustdep_dirs.push_back(
-        nonrustdep.AsSourceFile(settings_->build_settings()).GetDir());
-  }
-  for (const auto& nonrustdep_dir : nonrustdep_dirs) {
-    out_ << " -Lnative=";
-    path_output_.WriteDir(out_, nonrustdep_dir, PathOutput::DIR_NO_LAST_SLASH);
-  }
-
   // If rustc will invoke a linker, then pass linker arguments to include those
   // non-Rust native dependencies in the linking step.
 

Does this patch break Fuchsia or is it already relying on lib_dirs for any `#[link]` targets too?

Any other concerns? Did I successfully explain the problem, or can I explain anything better?

Thanks,

Dana

[David Turner]

Hello Dana,

Fuchsia build TL here, if you upload a CL with your patch, I'd be happy to build it and test all Fuchsia build configurations with it.

I am myself not in favor of `-Lnative=<dir>` due to the reasons explained in your email (the wrong libraries being picked up), so I am eager to fix whatever is needed in the Fuchsia build rules to get rid of them.

Thanks,

- Digit

To unsubscribe from this group and stop receiving emails from it, send an email to gn-dev+un...@chromium.org.

[danakj]

Thanks David.

A quick CL is here: https://gn-review.googlesource.com/c/gn/+/17300

It won't pass gn_unittests tests that are expecting the -Lnative directives, but it should let you try it out.

Cheers,

Dana