extra-link-arg-etc: support all link types (credit @davidhewitt)

[aviramha]

This commit adds support for the remaining link types to -Zextra-link-arg-etc:
rustc-link-arg-tests rustc-link-arg-benches rustc-link-arg-examples
This would be useful in PyO3, where users writing Python extension modules (which do link against libpython) want to run cargo tests for extension module. As executables, these tests need to link against libpython.

This is a follow up of #9416 by @davidhewitt

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

[ehuss]

Thanks for the PR! I'll try to get to review this when I can, but my queue is quite large right now.

A question though: Can you explain the use case in a little more detail? I'm curious why cargo:rustc-link-arg is not sufficient. Why do you need independent control over the different target types?

[aviramha]

The use case originates from PyO3's issue: PyO3/pyo3#1123
We want to be able to set link arguments when compiling tests specifically so cargo test would work correctly.

[ehuss]

Can you summarize why you need different link settings for tests? From a quick peek at that PR, I have no idea what it is doing. Please assume I don't know what PyO3 is or what you are trying to accomplish.

[aviramha]

Sorry for being vague. I wasn't sure how deep to go with the explanation.

PyO3 is a library and framework to write and build native Python extensions using Rust.

When building a library/extension (cdylib/so), we need to use dynamic linking to the library, so it would work on any environment.
When building tests/binaries, we need to link statically to libpython so those can run.

[ehuss]

Sounds good. I think this should be fine to add without the unstable flag. Would you mind removing that, and just updating the Build scripts chapter for the new options?

[aviramha]

@ehuss Done.

[ehuss]

Can these include validation that the package actually has a test or bench or example? Similar to rustc-link-arg-bins, this is intended to help the developer know that the instruction isn't valid. And if you can share the code between the bins/tests/benches/examples cases, that would be great!

[ehuss]

Can you also add these to the list at the top of the chapter?

[ehuss]

I think this [link-arg] definition can be removed. There only needs to be one in the file (Markdown only looks at one of them, ignoring the rest). Probably can remove the other duplicates, too.

[ehuss]

These calls to masquerade_as_nightly_cargo can be removed.

[aviramha]

@ehuss I've done the requested changes. I'm a bit "macrophobic' so any enhancements there would be welcome.

[davidhewitt]

Hey, just wanted to say thanks @aviramha for writing this and @ehuss / @joshtriplett for reviewing! PyO3's build model is somewhat complex thanks to Python, so having these flags will be a game-changer for downstream projects!

The explanation above wasn't quite right, so for posterity here's my understanding:

• When building a Python native extension (cdylib/so), we're not allowed to link to libpython; instead the Python interpreter provides the symbols we need at link load time.
• When building tests/binaries, we need to link dynamically to libpython so those can run their main. (or statically, but linking the interpreter statically is hard for other reasons not worth elaborating on here)

The fact that native extensions aren't allowed to link at all has been the gotcha for us in PyO3 - we have to resort to not linking any artifacts to libpython if users are building a native extension. This creates problems for users trying to cargo run or cargo test accompanying binaries in such a project, as they end up with unresolved symbols. With these flags we will have a mechanism to enable linking to libpython for these accompanying binaries, which will improve the experience of users we have using PyO3 (often Python users writing their first Rust).

[ehuss]

Thanks!

@rfcbot fcp merge

[rfcbot]

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @alexcrichton
• @ehuss
• @joshtriplett

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[alexcrichton]

Could this perhaps be a function instead of a macro?

(it'd be great to avoid 9-argument things with closures that close over outer values too)

[aviramha]

I chose macro over function as it might be a tedious to pass all the required parameters to it. I understand your concern though. If you think it's better as function I'll change it.

[aviramha]

I wonder what you'd expect the function to do and what would be it's signature? We can't pass the check function (as it's bound to a single instance?)

[alexcrichton]

Without being the one writing this I don't know precisely what it would look like. I find myself very rarely needing 9-parameter things and often in need of a refactoring of such a need arises.

[aviramha]

I tried playing with it a bit. Couldn't think of any better way to avoid the code duplication.
If you think it's better to revert to the old style (a bit of repetition) then I'm okay with it.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[aviramha]

Erm.. I'm not quite sure what's the state right now? @ehuss

[ehuss]

I think it would be good to simplify it some.

Macros can capture from their local environment. If it is defined within the loop, I think it could be something like this:

diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs
index bc8167633..a89984dd0 100644
--- a/src/cargo/core/compiler/custom_build.rs
+++ b/src/cargo/core/compiler/custom_build.rs
@@ -17,22 +17,6 @@ use std::sync::{Arc, Mutex};

 const CARGO_WARNING: &str = "cargo:warning=";

-macro_rules! check_and_add_target {
-    ($targets:ident, $func: ident, $key: ident, $whence: ident, $pkg_descr: ident, $link_type: expr, $value: ident, $type_name: literal, $linker_args: ident) => {
-        if !$targets.iter().any(|target| target.$func()) {
-            bail!(
-                "invalid instruction `cargo:{}` from {}\n\
-                    The package {} does not have a {} target.",
-                $key,
-                $whence,
-                $pkg_descr,
-                $type_name
-            );
-        }
-        $linker_args.push(($link_type, $value));
-    };
-}
-
 /// Contains the parsed output of a custom build script.
 #[derive(Clone, Debug, Hash, Default)]
 pub struct BuildOutput {
@@ -595,6 +579,22 @@ impl BuildOutput {
                 script_out_dir.to_str().unwrap(),
             );

+            macro_rules! check_and_add {
+                ($target_kind: expr, $is_target_kind: expr, $link_type: expr) => {
+                    if !targets.iter().any(|target| $is_target_kind(target)) {
+                        bail!(
+                            "invalid instruction `cargo:{}` from {}\n\
+                                The package {} does not have a {} target.",
+                            key,
+                            whence,
+                            pkg_descr,
+                            $target_kind
+                        );
+                    }
+                    linker_args.push(($link_type, value));
+                };
+            }
+
             // Keep in sync with TargetConfig::parse_links_overrides.
             match key {
                 "rustc-flags" => {
@@ -620,17 +620,7 @@ impl BuildOutput {
                     linker_args.push((LinkType::Cdylib, value))
                 }
                 "rustc-link-arg-bins" => {
-                    check_and_add_target!(
-                        targets,
-                        is_bin,
-                        key,
-                        whence,
-                        pkg_descr,
-                        LinkType::Bin,
-                        value,
-                        "bin",
-                        linker_args
-                    );
+                    check_and_add!("bin", Target::is_bin, LinkType::Bin);
                 }
                 "rustc-link-arg-bin" => {
                     let mut parts = value.splitn(2, '=');
@@ -661,43 +651,13 @@ impl BuildOutput {
                     linker_args.push((LinkType::SingleBin(bin_name), arg.to_string()));
                 }
                 "rustc-link-arg-tests" => {
-                    check_and_add_target!(
-                        targets,
-                        is_test,
-                        key,
-                        whence,
-                        pkg_descr,
-                        LinkType::Test,
-                        value,
-                        "test",
-                        linker_args
-                    );
+                    check_and_add!("test", Target::is_test, LinkType::Test);
                 }
                 "rustc-link-arg-benches" => {
-                    check_and_add_target!(
-                        targets,
-                        is_bench,
-                        key,
-                        whence,
-                        pkg_descr,
-                        LinkType::Bench,
-                        value,
-                        "benchmark",
-                        linker_args
-                    );
+                    check_and_add!("benchmark", Target::is_bench, LinkType::Bench);
                 }
                 "rustc-link-arg-examples" => {
-                    check_and_add_target!(
-                        targets,
-                        is_example,
-                        key,
-                        whence,
-                        pkg_descr,
-                        LinkType::Example,
-                        value,
-                        "example",
-                        linker_args
-                    );
+                    check_and_add!("example", Target::is_example, LinkType::Example);
                 }
                 "rustc-link-arg" => {
                     linker_args.push((LinkType::All, value));

Generally I would prefer using a closure. However, in this case that would require passing a few more things in due to the way closures capture things. Overall the macro seems a little simpler, though not as elegant.

[aviramha]

@ehuss I've applied your change. Thanks!
I wasn't sure if macro can be defined in function scope, apparently it can.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[ehuss]

Thanks!

@bors r+

[bors]

📌 Commit 541c908 has been approved by ehuss

[bors]

⌛ Testing commit 541c908 with merge b2496f0...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing b2496f0 to master...