Improve handling of build.rs link dependencies (#1970)

This PR helps clarify the difference between cargo_build_script build
dependencies, and link dependencies. It separates the two, and ensures
that link_deps are only for direct (non-transitive) normal dependencies
of the build crate. It does not fix the issue with how crates universe
detects links crates although it does add a comment explaining the
issue.

cargo/private/cargo_build_script.bzl

@@ -225,14 +225,18 @@ def _cargo_build_script_impl(ctx):
         streams.stderr.path,
     ])
     build_script_inputs = []
-    for dep in ctx.attr.deps:
+    for dep in ctx.attr.link_deps:
         if rust_common.dep_info in dep and dep[rust_common.dep_info].dep_env:
             dep_env_file = dep[rust_common.dep_info].dep_env
             args.add(dep_env_file.path)
             build_script_inputs.append(dep_env_file)
             for dep_build_info in dep[rust_common.dep_info].transitive_build_infos.to_list():
                 build_script_inputs.append(dep_build_info.out_dir)
 
+    for dep in ctx.attr.deps:
+        for dep_build_info in dep[rust_common.dep_info].transitive_build_infos.to_list():
+            build_script_inputs.append(dep_build_info.out_dir)
+
     if feature_enabled(ctx, SYMLINK_EXEC_ROOT_FEATURE):
         env["RULES_RUST_SYMLINK_EXEC_ROOT"] = "1"
 
@@ -280,7 +284,16 @@ cargo_build_script = rule(
             allow_files = True,
         ),
         "deps": attr.label_list(
-            doc = "The Rust dependencies of the crate",
+            doc = "The Rust build-dependencies of the crate",
+            providers = [rust_common.dep_info],
+            cfg = "exec",
+        ),
+        "link_deps": attr.label_list(
+            doc = dedent("""\
+                The subset of the Rust (normal) dependencies of the crate that
+                have the links attribute and therefore provide environment
+                variables to this build script.
+            """),
             providers = [rust_common.dep_info],
             cfg = "exec",
         ),

cargo/private/cargo_build_script_wrapper.bzl

@@ -13,6 +13,7 @@ def cargo_build_script(
         crate_features = [],
         version = None,
         deps = [],
+        link_deps = [],
         build_script_env = {},
         data = [],
         tools = [],
@@ -85,7 +86,9 @@ def cargo_build_script(
             being compiled, optionally with a suffix of `_build_script`.
         crate_features (list, optional): A list of features to enable for the build script.
         version (str, optional): The semantic version (semver) of the crate.
-        deps (list, optional): The dependencies of the crate.
+        deps (list, optional): The build-dependencies of the crate.
+        link_deps (list, optional): The subset of the (normal) dependencies of the crate that have the
+            links attribute and therefore provide environment variables to this build script.
         build_script_env (dict, optional): Environment variables for build scripts.
         data (list, optional): Files needed by the build script.
         tools (list, optional): Tools (executables) needed by the build script.
@@ -144,6 +147,7 @@ def cargo_build_script(
         build_script_env = build_script_env,
         links = links,
         deps = deps,
+        link_deps = link_deps,
         data = data,
         tools = tools,
         rustc_flags = rustc_flags,

crate_universe/src/context/crate_context.rs

@@ -203,6 +203,29 @@ pub struct BuildScriptAttributes {
     #[serde(skip_serializing_if = "BTreeSet::is_empty")]
     pub extra_deps: BTreeSet<String>,
 
+    // TODO: refactor a crate with a build.rs file from two into three bazel
+    // rules in order to deduplicate link_dep information. Currently as the
+    // crate depends upon the build.rs file, the build.rs cannot find the
+    // information for the normal dependencies of the crate. This could be
+    // solved by switching the dependency graph from:
+    //
+    //   rust_library -> cargo_build_script
+    //
+    // to:
+    //
+    //   rust_library ->-+-->------------------->--+
+    //                   |                         |
+    //                   +--> cargo_build_script --+--> crate dependencies
+    //
+    // in which either all of the deps are in crate dependencies, or just the
+    // normal dependencies. This could be handled a special rule, or just using
+    // a `filegroup`.
+    #[serde(skip_serializing_if = "SelectList::is_empty")]
+    pub link_deps: SelectList<CrateDependency>,
+
+    #[serde(skip_serializing_if = "BTreeSet::is_empty")]
+    pub extra_link_deps: BTreeSet<String>,
+
     #[serde(skip_serializing_if = "SelectStringDict::is_empty")]
     pub build_script_env: SelectStringDict,
 
@@ -240,6 +263,8 @@ impl Default for BuildScriptAttributes {
             data_glob: BTreeSet::from(["**".to_owned()]),
             deps: Default::default(),
             extra_deps: Default::default(),
+            link_deps: Default::default(),
+            extra_link_deps: Default::default(),
             build_script_env: Default::default(),
             extra_proc_macro_deps: Default::default(),
             proc_macro_deps: Default::default(),
@@ -411,6 +436,7 @@ impl CrateContext {
             );
 
             let build_deps = annotation.deps.build_deps.clone().map(new_crate_dep);
+            let build_link_deps = annotation.deps.build_link_deps.clone().map(new_crate_dep);
             let build_proc_macro_deps = annotation
                 .deps
                 .build_proc_macro_deps
@@ -419,6 +445,7 @@ impl CrateContext {
 
             Some(BuildScriptAttributes {
                 deps: build_deps,
+                link_deps: build_link_deps,
                 proc_macro_deps: build_proc_macro_deps,
                 links: package.links.clone(),
                 ..Default::default()

crate_universe/src/metadata/dependency.rs

@@ -29,6 +29,7 @@ pub struct DependencySet {
     pub proc_macro_deps: SelectList<Dependency>,
     pub proc_macro_dev_deps: SelectList<Dependency>,
     pub build_deps: SelectList<Dependency>,
+    pub build_link_deps: SelectList<Dependency>,
     pub build_proc_macro_deps: SelectList<Dependency>,
 }
 
@@ -69,7 +70,7 @@ impl DependencySet {
 
         // For rules on build script dependencies see:
         //  https://doc.rust-lang.org/cargo/reference/build-scripts.html#build-dependencies
-        let (build_proc_macro_deps, mut build_deps) = {
+        let (build_proc_macro_deps, build_deps) = {
             let (proc_macro, normal) = node
                 .deps
                 .iter()
@@ -85,24 +86,24 @@ impl DependencySet {
             )
         };
 
-        // `*-sys` packages follow slightly different rules than other dependencies. These
-        // packages seem to provide some environment variables required to build the top level
-        // package and are expected to be avialable to other build scripts. If a target depends
-        // on a `*-sys` crate for itself, so would it's build script. Hopefully this is correct.
+        // packages with the `links` property follow slightly different rules than other
+        // dependencies. These packages provide zero or more environment variables to the build
+        // script's of packages that directly (non-transitively) depend on these packages. Note that
+        // dependency specifically means of the package (`dependencies`), and not of the build
+        // script (`build-dependencies`).
         // https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key
         // https://doc.rust-lang.org/cargo/reference/build-scripts.html#-sys-packages
         // https://doc.rust-lang.org/cargo/reference/build-script-examples.html#using-another-sys-crate
-        let sys_name = format!("{}-sys", &metadata[&node.id].name);
+        let mut build_link_deps = SelectList::<Dependency>::default();
         normal_deps.configurations().into_iter().for_each(|config| {
             normal_deps
                 .get_iter(config)
                 // Iterating over known key should be safe
                 .unwrap()
                 // Add any normal dependency to build dependencies that are associated `*-sys` crates
                 .for_each(|dep| {
-                    let dep_pkg_name = &metadata[&dep.package_id].name;
-                    if *dep_pkg_name == sys_name {
-                        build_deps.insert(dep.clone(), config.cloned())
+                    if metadata[&dep.package_id].links.is_some() {
+                        build_link_deps.insert(dep.clone(), config.cloned())
                     }
                 });
         });
@@ -113,6 +114,7 @@ impl DependencySet {
             proc_macro_deps,
             proc_macro_dev_deps,
             build_deps,
+            build_link_deps,
             build_proc_macro_deps,
         }
     }
@@ -430,7 +432,7 @@ mod test {
 
         let dependencies = DependencySet::new_for_node(openssl_node, &metadata);
 
-        let sys_crate = dependencies
+        let normal_sys_crate = dependencies
             .normal_deps
             .get_iter(None)
             .unwrap()
@@ -439,9 +441,19 @@ mod test {
                 pkg.name == "openssl-sys"
             });
 
+        let link_dep_sys_crate = dependencies
+            .build_link_deps
+            .get_iter(None)
+            .unwrap()
+            .find(|dep| {
+                let pkg = &metadata[&dep.package_id];
+                pkg.name == "openssl-sys"
+            });
+
         // sys crates like `openssl-sys` should always be dependencies of any
         // crate which matches it's name minus the `-sys` suffix
-        assert!(sys_crate.is_some());
+        assert!(normal_sys_crate.is_some());
+        assert!(link_dep_sys_crate.is_some());
     }
 
     #[test]

crate_universe/src/rendering.rs

@@ -365,6 +365,12 @@ impl Renderer {
                     attrs.map_or(&empty_set, |attrs| &attrs.extra_deps),
                 )
                 .remap_configurations(platforms),
+            link_deps: self
+                .make_deps(
+                    attrs.map_or(&empty_deps, |attrs| &attrs.link_deps),
+                    attrs.map_or(&empty_set, |attrs| &attrs.extra_link_deps),
+                )
+                .remap_configurations(platforms),
             edition: krate.common_attrs.edition.clone(),
             linker_script: krate.common_attrs.linker_script.clone(),
             links: attrs.and_then(|attrs| attrs.links.clone()),

crate_universe/src/utils/starlark.rs

@@ -99,6 +99,11 @@ pub struct CargoBuildScript {
         serialize_with = "SelectList::serialize_starlark"
     )]
     pub deps: SelectList<WithOriginalConfigurations<String>>,
+    #[serde(
+        skip_serializing_if = "SelectList::is_empty",
+        serialize_with = "SelectList::serialize_starlark"
+    )]
+    pub link_deps: SelectList<WithOriginalConfigurations<String>>,
     pub edition: String,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub linker_script: Option<String>,

docs/cargo.md

@@ -64,8 +64,8 @@ A rule for generating variables for dependent `cargo_build_script`s without a bu
 ## cargo_build_script
 
 <pre>
-cargo_build_script(<a href="#cargo_build_script-name">name</a>, <a href="#cargo_build_script-crate_features">crate_features</a>, <a href="#cargo_build_script-version">version</a>, <a href="#cargo_build_script-deps">deps</a>, <a href="#cargo_build_script-build_script_env">build_script_env</a>, <a href="#cargo_build_script-data">data</a>, <a href="#cargo_build_script-tools">tools</a>, <a href="#cargo_build_script-links">links</a>,
-                   <a href="#cargo_build_script-rustc_env">rustc_env</a>, <a href="#cargo_build_script-rustc_flags">rustc_flags</a>, <a href="#cargo_build_script-visibility">visibility</a>, <a href="#cargo_build_script-tags">tags</a>, <a href="#cargo_build_script-kwargs">kwargs</a>)
+cargo_build_script(<a href="#cargo_build_script-name">name</a>, <a href="#cargo_build_script-crate_features">crate_features</a>, <a href="#cargo_build_script-version">version</a>, <a href="#cargo_build_script-deps">deps</a>, <a href="#cargo_build_script-link_deps">link_deps</a>, <a href="#cargo_build_script-build_script_env">build_script_env</a>, <a href="#cargo_build_script-data">data</a>, <a href="#cargo_build_script-tools">tools</a>,
+                   <a href="#cargo_build_script-links">links</a>, <a href="#cargo_build_script-rustc_env">rustc_env</a>, <a href="#cargo_build_script-rustc_flags">rustc_flags</a>, <a href="#cargo_build_script-visibility">visibility</a>, <a href="#cargo_build_script-tags">tags</a>, <a href="#cargo_build_script-kwargs">kwargs</a>)
 </pre>
 
 Compile and execute a rust build script to generate build attributes
@@ -134,7 +134,8 @@ The `hello_lib` target will be build with the flags and the environment variable
 | <a id="cargo_build_script-name"></a>name |  The name for the underlying rule. This should be the name of the package being compiled, optionally with a suffix of <code>_build_script</code>.   |  none |
 | <a id="cargo_build_script-crate_features"></a>crate_features |  A list of features to enable for the build script.   |  `[]` |
 | <a id="cargo_build_script-version"></a>version |  The semantic version (semver) of the crate.   |  `None` |
-| <a id="cargo_build_script-deps"></a>deps |  The dependencies of the crate.   |  `[]` |
+| <a id="cargo_build_script-deps"></a>deps |  The build-dependencies of the crate.   |  `[]` |
+| <a id="cargo_build_script-link_deps"></a>link_deps |  The subset of the (normal) dependencies of the crate that have the links attribute and therefore provide environment variables to this build script.   |  `[]` |
 | <a id="cargo_build_script-build_script_env"></a>build_script_env |  Environment variables for build scripts.   |  `{}` |
 | <a id="cargo_build_script-data"></a>data |  Files needed by the build script.   |  `[]` |
 | <a id="cargo_build_script-tools"></a>tools |  Tools (executables) needed by the build script.   |  `[]` |

docs/flatten.md

@@ -1539,8 +1539,8 @@ A collection of files either found within the `rust-stdlib` artifact or generate
 ## cargo_build_script
 
 <pre>
-cargo_build_script(<a href="#cargo_build_script-name">name</a>, <a href="#cargo_build_script-crate_features">crate_features</a>, <a href="#cargo_build_script-version">version</a>, <a href="#cargo_build_script-deps">deps</a>, <a href="#cargo_build_script-build_script_env">build_script_env</a>, <a href="#cargo_build_script-data">data</a>, <a href="#cargo_build_script-tools">tools</a>, <a href="#cargo_build_script-links">links</a>,
-                   <a href="#cargo_build_script-rustc_env">rustc_env</a>, <a href="#cargo_build_script-rustc_flags">rustc_flags</a>, <a href="#cargo_build_script-visibility">visibility</a>, <a href="#cargo_build_script-tags">tags</a>, <a href="#cargo_build_script-kwargs">kwargs</a>)
+cargo_build_script(<a href="#cargo_build_script-name">name</a>, <a href="#cargo_build_script-crate_features">crate_features</a>, <a href="#cargo_build_script-version">version</a>, <a href="#cargo_build_script-deps">deps</a>, <a href="#cargo_build_script-link_deps">link_deps</a>, <a href="#cargo_build_script-build_script_env">build_script_env</a>, <a href="#cargo_build_script-data">data</a>, <a href="#cargo_build_script-tools">tools</a>,
+                   <a href="#cargo_build_script-links">links</a>, <a href="#cargo_build_script-rustc_env">rustc_env</a>, <a href="#cargo_build_script-rustc_flags">rustc_flags</a>, <a href="#cargo_build_script-visibility">visibility</a>, <a href="#cargo_build_script-tags">tags</a>, <a href="#cargo_build_script-kwargs">kwargs</a>)
 </pre>
 
 Compile and execute a rust build script to generate build attributes
@@ -1609,7 +1609,8 @@ The `hello_lib` target will be build with the flags and the environment variable
 | <a id="cargo_build_script-name"></a>name |  The name for the underlying rule. This should be the name of the package being compiled, optionally with a suffix of <code>_build_script</code>.   |  none |
 | <a id="cargo_build_script-crate_features"></a>crate_features |  A list of features to enable for the build script.   |  `[]` |
 | <a id="cargo_build_script-version"></a>version |  The semantic version (semver) of the crate.   |  `None` |
-| <a id="cargo_build_script-deps"></a>deps |  The dependencies of the crate.   |  `[]` |
+| <a id="cargo_build_script-deps"></a>deps |  The build-dependencies of the crate.   |  `[]` |
+| <a id="cargo_build_script-link_deps"></a>link_deps |  The subset of the (normal) dependencies of the crate that have the links attribute and therefore provide environment variables to this build script.   |  `[]` |
 | <a id="cargo_build_script-build_script_env"></a>build_script_env |  Environment variables for build scripts.   |  `{}` |
 | <a id="cargo_build_script-data"></a>data |  Files needed by the build script.   |  `[]` |
 | <a id="cargo_build_script-tools"></a>tools |  Tools (executables) needed by the build script.   |  `[]` |

test/dep_env/BUILD.bazel

@@ -11,6 +11,14 @@ cargo_build_script(
     links = "X",
 )
 
+cargo_build_script(
+    name = "set_c_build",
+    srcs = ["set_c.rs"],
+    edition = "2018",
+    link_deps = [":set_a"],
+    links = "Y",
+)
+
 rust_library(
     name = "set_a",
     srcs = ["empty.rs"],
@@ -23,25 +31,43 @@ cargo_dep_env(
     src = "set_b.env",
 )
 
+rust_library(
+    name = "set_c",
+    srcs = ["empty.rs"],
+    edition = "2018",
+    deps = [
+        ":set_a",
+        ":set_c_build",
+    ],
+)
+
 cargo_build_script(
     name = "read_a",
     srcs = ["read_a.rs"],
     edition = "2018",
-    deps = [":set_a"],
+    link_deps = [":set_a"],
 )
 
 cargo_build_script(
     name = "read_b",
     srcs = ["read_b.rs"],
     edition = "2018",
-    deps = [":set_b"],
+    link_deps = [":set_b"],
+)
+
+cargo_build_script(
+    name = "read_c",
+    srcs = ["read_c.rs"],
+    edition = "2018",
+    link_deps = [":set_c"],
+    deps = [":set_a"],
 )
 
 cargo_build_script(
     name = "read_dep_dir",
     srcs = ["read_dep_dir.rs"],
     edition = "2018",
-    deps = [":set_dep_dir"],
+    link_deps = [":set_dep_dir"],
 )
 
 rust_test(
@@ -58,6 +84,13 @@ rust_test(
     deps = [":read_b"],
 )
 
+rust_test(
+    name = "build_read_c",
+    srcs = ["read_c.rs"],
+    edition = "2018",
+    deps = [":read_c"],
+)
+
 rust_test(
     name = "build_read_dep_dir",
     srcs = ["read_dep_dir.rs"],

test/dep_env/read_c.rs

@@ -0,0 +1,6 @@
+use std::env::var;
+
+fn main() {
+    assert!(var("DEP_X_A").is_err());
+    assert_eq!(var("DEP_Y_C").unwrap(), "c_value");
+}

test/dep_env/set_c.rs

@@ -0,0 +1,3 @@
+fn main() {
+    println!("cargo:C=c_value");
+}