Finish implementation of `-Zextra-link-arg`. #8441

reitermarkus · 2020-07-02T10:59:02Z

This is a continuation of #7811, which adds tests and a warning if the -Zextra-link-arg flag is not specified. Also moved the documentation into unstable.md.

rust-highfive · 2020-07-02T10:59:05Z

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

bors · 2020-07-09T16:46:22Z

☔ The latest upstream changes (presumably #8427) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2020-07-09T18:25:04Z

Are arguments passed with rustc-link-arg (without a crate type) passed when building dylibs or proc-macros?
These crate types also have a linking step, but may want to use a different (perhaps empty) set of flags.

ehuss · 2020-07-10T02:41:07Z

Thanks for picking this back up @reitermarkus! I just wanted to let you know that this is in my queue, and I'll try to get to it soon. I think there was still an open issue of how the syntax can be extended for setting args for different targets, and I'd like to discuss that with the team some more.

ehuss · 2020-07-15T21:12:45Z

Sorry about the delay.

The main stickler on this was just making sure that the syntax for specifying which targets get the flags for different scenarios. I'd like to make a proposal to use something like the following structure:

cargo:rustc-link-arg-bin-NAME=…      # equivalent to --bin=NAME
cargo:rustc-link-arg-example-NAME=…  # equivalent to --example=NAME
cargo:rustc-link-arg-bins=…          # equivalent to --bins
cargo:rustc-link-arg-examples=…      # equivalent to --examples
cargo:rustc-link-arg-test-NAME=…     # equivalent to --test=NAME
cargo:rustc-link-arg-tests=…         # equivalent to --tests (including unittests)
cargo:rustc-link-arg-unittests=…     # ??? (not sure what this means)
cargo:rustc-link-arg-test-LIBNAME=…  # for unittest of lib only?
cargo:rustc-link-arg-cdylib=…        # for cdylib
cargo:rustc-link-arg-bench-NAME=…    # equivalent to --bench=NAME
cargo:rustc-link-arg-benches=…       # equivalent to --benches
cargo:rustc-link-arg=…               # anything linkable? bins,examples,tests,benches,cdylib?

I'm not proposing that this PR needs to implement all of these, but that there is a long-term plan for how new targets can be added. This specifically has an impact on the bare rustc-link-arg since it would change its meaning from this PR.

For now I wouldn't bother renaming rustc-cdylib-link-arg, though in the future, for consistency we might want to change it (that is, accept both, and soft-deprecate the old syntax).

Does that seem like a reasonable path forward?

retep998 · 2020-07-15T22:05:24Z

@ehuss How would the syntax look for cdylib if #6351 was implemented?

ehuss · 2020-07-15T22:16:41Z

If that is implemented as a [[bin]] target, then I would it assume it would work like any bin target, specified with rustc-link-arg-bin-foo, or rustc-link-arg-bins, or rustc-link-arg depending on your needs.

reitermarkus · 2020-07-22T00:05:26Z

@ehuss, I renamed rustc-bin-link-arg to rustc-link-arg-bins and also made it accept rustc-link-arg-cdylib.

davidhewitt · 2020-07-23T18:50:14Z

Hi! I was just experimenting with this branch for pyo3, where we need to situationally link libpython depending on what the user is building. Python "extension modules" for distribution shouldn't link to libpython, whereas binaries will always need to be linked.

Currently this is solved by enabling a feature which switches off libpython linking, but that's a fairly blunt hammer that creates other problems - e.g. proc macros, tests and binaries will no longer compile. (PyO3/pyo3#771)

I was hoping that we might be able to use this feature to achieve finer-grain control over this issue. However, it looks like build script directives emitted in pyo3's build.rs only apply in the context of pyo3? i.e. rustc-link-arg-bins applies if I'm building a binary which is part of the pyo3 crate, but if I make a new crate which depends on pyo3, this child crate doesn't pick up the extra link arg.

Would it be within the scope of this feature to pass link args to dependencies? Or, can it be extended to do so?

In order to use the switchless feature of the SGX, the app binary needs to be linked with the following flags. ``` -Wl,--whole-archive -lsgx_uswitchless -Wl,--no-whole-archive ```` Noted that there is currently no proper way to pass custom link flags to all the binaries in a cargo project. As such, the `link_args` macro need to be set for each of Rust files with `main` methods. References: * Build flags used in Intel SGX sample code: https://github.com/intel/linux-sgx/blob/ce4a18d9a0963b14f717cc7462241cfadd233f55/SampleCode/Switchless/Makefile#L104 * Rust tracking issue on `link_args`: rust-lang/rust#29596 * PR for `cargo:rustc-link-arg-bins` in cargo build script: rust-lang/cargo#8441

ehuss · 2020-08-25T18:05:24Z

@davidhewitt It looks like dependencies that use rustc-link-arg-bins should pass those args to the binary. Can you put together a minimal repro showing that it does not?

Sorry again for the delay on this. I think this seems reasonable to move forward with.

@rust-lang/cargo This is an unstable feature which allows build scripts to specify link args for additional targets. cdylib is already supported on stable, but this adds unstable support for bins, and "all targets". If people need finer-grained target selection, the syntax suggested in #8441 (comment) could be used as needed. I'm going to FCP this, even though it is unstable, since we've been discussing it off and on.

@rfcbot merge

rfcbot · 2020-08-25T18:05:27Z

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

Concerns:

rustc-link-arg should match all linkable targets (Finish implementation of -Zextra-link-arg. #8441 (comment))

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

Looks reasonable to me, but I wanted to clarify something before signing off. Does "binary" here include binaries for --test? (or examples, benchmarks, etc) Or is it only the executables in src/bin/*.rs effectively?

Additionally does rustc-link-arg apply to all of the above, or only [[bin]] and cdylib? Does rustc-link-arg apply to dylib crate types too?

ehuss · 2020-08-25T18:45:58Z

rustc-link-arg-bins is just binary targets, those that are specified with [[bin]] or in the bins directory. This mirrors the --bins command-line flag.

rustc-link-arg is supposed to apply to everything "linkable", tests, bins, benchmarks, cdylib, etc. However, on closer inspection, I see that this PR does not do that. (Sorry, I thought I had tested that.)

@rfcbot concern rustc-link-arg should match all linkable targets

@reitermarkus do you think you could look at that?

alexcrichton · 2020-08-25T19:04:57Z

Ok yeah that sounds good to me and is what I would expect, could the documentation be updated here as well?

bors · 2020-11-18T21:44:13Z

📌 Commit 51f7a44 has been approved by alexcrichton

bors · 2020-11-18T21:44:23Z

⌛ Testing commit 51f7a44 with merge 9051345...

bors · 2020-11-18T22:14:23Z

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 9051345 to master...

Update cargo 10 commits in 2af662e22177a839763ac8fb70d245a680b15214..bfca1cd22bf514d5f2b6c1089b0ded0ba7dfaa6e 2020-11-12 19:04:56 +0000 to 2020-11-24 16:33:21 +0000 - Shrink the progress bar, to give more space after it. (rust-lang/cargo#8892) - Add some comments to the toml code (rust-lang/cargo#8887) - Start searching git config at new path (rust-lang/cargo#8886) - Fix documentation for CARGO_PRIMARY_PACKAGE. (rust-lang/cargo#8891) - Bump to 0.51.0, update changelog (rust-lang/cargo#8894) - Publish target's "doc" setting when emitting metadata (rust-lang/cargo#8869) - Relaxes expectation of `cargo test` tests to accept test execution time (rust-lang/cargo#8884) - Finish implementation of `-Zextra-link-arg`. (rust-lang/cargo#8441) - Reproducible crate builds (rust-lang/cargo#8864) - Allow resolver="1" to explicitly use the old resolver behavior. (rust-lang/cargo#8857)

jstarks · 2021-04-25T22:49:30Z

Is there a tracking issue for stabilizing this?

ehuss · 2021-04-28T21:25:29Z

@jstarks I opened #9426 to track this.

Change `rustc-cdylib-link-arg` error to a warning. In #9523, an error was added if `cargo:rustc-cdylib-link-arg` was issued in a build script without actually having a cdylib target. This uncovered that there was an unintentional change in #8441 to cause those link args to be applied to transitive dependencies. This changes it so that the error is now a warning, with a note that this may become an error in the future. It also changes it so that the unstable `rustc-link-arg*` instructions only apply to the package that emitted them.

Change `rustc-cdylib-link-arg` error to a warning. In rust-lang#9523, an error was added if `cargo:rustc-cdylib-link-arg` was issued in a build script without actually having a cdylib target. This uncovered that there was an unintentional change in rust-lang#8441 to cause those link args to be applied to transitive dependencies. This changes it so that the error is now a warning, with a note that this may become an error in the future. It also changes it so that the unstable `rustc-link-arg*` instructions only apply to the package that emitted them.

Stabilize the rustc-link-arg option This change removes the unstable option (tracked by #9426) and unconditionally accepts additional linker arguments (as implemented in #7811 and #8441). Documentation is moved from unstable to what appeared to be the correct location. I am not aware of any significant concerns with the option and it appears consistent with some other existing stable linker options. Please let me know if this is not the appropriate process or if there is anything that I am missing from the PR.

Enable triagebot's relabel functionality ### What does this PR try to resolve? This fixes the following failure that rustbot currently posts whenever someone tries to use "`@rustbot` label" in this repository. > **Error**: The feature `relabel` is not enabled in this repository. > To enable it add its section in the `triagebot.toml` in the root of the repository. Unauthenticated relabel has been enabled in rust-lang/rust for nearly 4 years. People overwhelmingly use it in good faith. ### How should we test and review this PR? Compare against https://github.com/rust-lang/rust/blob/1.66.0/triagebot.toml. Also skim through the 7 pages of labels on https://github.com/rust-lang/cargo/labels, whether it makes sense the ones I decided to allow arbitrary GitHub users to apply. ### Additional information Attempted uses of "`@rustbot` label", that failed, but this PR would allow: - #10343 (comment) - #10243 (comment) - #9982 (comment) - #9128 (comment) - #9067 (comment) - #8441 (comment) - #11432 (comment) - #8841 (comment) - #10820 (comment) - #10572 (comment) - #9114 (comment) - #8980 (comment) - #9064 (comment) - #8726 (comment) - #8089 (comment)

rust-highfive assigned ehuss Jul 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2020

reitermarkus mentioned this pull request Jul 2, 2020

Add rustc-bin-link-arg and rustc-link-arg custom build options #7811

Closed

petrochenkov mentioned this pull request Jul 9, 2020

Check for proc_macro before -Ctarget-features=+crt-static rust-lang/rust#74179

Closed

davidhewitt mentioned this pull request Jul 14, 2020

Replace extension-module feature with PYO3_MAKE_EXTENSION_MODULE PyO3/pyo3#1040

Closed

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2020

reitermarkus force-pushed the linkarg branch from 9f0ee73 to 346334d Compare July 21, 2020 23:56

reitermarkus force-pushed the linkarg branch from b60fb6a to 61fce9e Compare July 22, 2020 00:19

reitermarkus force-pushed the linkarg branch from 61fce9e to 6382d8b Compare July 24, 2020 06:55

xu-cheng mentioned this pull request Aug 1, 2020

Fix switchless sample code apache/incubator-teaclave-sgx-sdk#260

Merged

ehuss added T-cargo Team: Cargo and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Aug 25, 2020

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 25, 2020

alexcrichton reviewed Aug 25, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2020

bors merged commit 9051345 into rust-lang:master Nov 18, 2020

This was referenced Nov 18, 2020

Propagate linker args upwards in the dependency graph. #7612

Closed

Implement future incompatibility report support #8825

Merged

reitermarkus deleted the linkarg branch November 19, 2020 05:40

luojia65 added a commit to gd32v-rust/gd32vf103-hal that referenced this pull request Nov 24, 2020

Update for extra-link-arg (rust-lang/cargo#8441)

1b3af0e

ehuss mentioned this pull request Nov 24, 2020

Update cargo rust-lang/rust#79384

Merged

LaurentMazare mentioned this pull request Nov 26, 2020

Cuda not available in cargo run --release (BUG)? LaurentMazare/tch-rs#291

Open

stdrc mentioned this pull request Jan 31, 2021

Allow adding a linker script in build.rs #7984

Closed

rubdos added a commit to whisperfish/whisperfish that referenced this pull request Feb 10, 2021

Prepare for -Zextra-link-arg rust-lang/cargo#8441

05c8580

ehuss mentioned this pull request Apr 28, 2021

Tracking Issue for extra-link-arg #9426

Closed

danielframpton mentioned this pull request Jun 8, 2021

Stabilize the rustc-link-arg option #9557

Merged

ehuss mentioned this pull request Jun 9, 2021

Change rustc-cdylib-link-arg error to a warning. #9563

Merged

mattiekat mentioned this pull request Jun 13, 2021

per-package-target looks in project root for memory.x rather then crate specific directories #9537

Open

hydra mentioned this pull request Sep 15, 2021

Example doesn't work. xobs/rust-extra-link-arg-test#1

Open

XieJiSS mentioned this pull request Dec 11, 2021

Support rustc-link-args in order to properly pass -pthread and flags #7796

Closed

ehuss added this to the 1.50.0 milestone Feb 6, 2022

ehuss mentioned this pull request Aug 13, 2022

cargo:rustc-link-arg-tests not available to unit/module tests #10937

Open

dtolnay mentioned this pull request Dec 18, 2022

Enable triagebot's relabel functionality #11498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish implementation of `-Zextra-link-arg`. #8441

Finish implementation of `-Zextra-link-arg`. #8441

reitermarkus commented Jul 2, 2020

rust-highfive commented Jul 2, 2020

bors commented Jul 9, 2020

petrochenkov commented Jul 9, 2020

ehuss commented Jul 10, 2020

ehuss commented Jul 15, 2020

retep998 commented Jul 15, 2020

ehuss commented Jul 15, 2020

reitermarkus commented Jul 22, 2020

davidhewitt commented Jul 23, 2020

ehuss commented Aug 25, 2020

rfcbot commented Aug 25, 2020 •

edited by alexcrichton

Loading

alexcrichton left a comment

ehuss commented Aug 25, 2020

alexcrichton commented Aug 25, 2020

bors commented Nov 18, 2020

bors commented Nov 18, 2020

bors commented Nov 18, 2020

jstarks commented Apr 25, 2021

ehuss commented Apr 28, 2021

Finish implementation of -Zextra-link-arg. #8441

Finish implementation of -Zextra-link-arg. #8441

Conversation

reitermarkus commented Jul 2, 2020

rust-highfive commented Jul 2, 2020

bors commented Jul 9, 2020

petrochenkov commented Jul 9, 2020

ehuss commented Jul 10, 2020

ehuss commented Jul 15, 2020

retep998 commented Jul 15, 2020

ehuss commented Jul 15, 2020

reitermarkus commented Jul 22, 2020

davidhewitt commented Jul 23, 2020

ehuss commented Aug 25, 2020

rfcbot commented Aug 25, 2020 • edited by alexcrichton Loading

alexcrichton left a comment

Choose a reason for hiding this comment

ehuss commented Aug 25, 2020

alexcrichton commented Aug 25, 2020

bors commented Nov 18, 2020

bors commented Nov 18, 2020

bors commented Nov 18, 2020

jstarks commented Apr 25, 2021

ehuss commented Apr 28, 2021

Finish implementation of `-Zextra-link-arg`. #8441

Finish implementation of `-Zextra-link-arg`. #8441

rfcbot commented Aug 25, 2020 •

edited by alexcrichton

Loading