Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue for profile-rustflags #10271

Open
5 tasks
ehuss opened this issue Jan 7, 2022 · 23 comments
Open
5 tasks

Tracking Issue for profile-rustflags #10271

ehuss opened this issue Jan 7, 2022 · 23 comments
Labels
A-profiles Area: profiles A-rustflags Area: rustflags C-tracking-issue Category: A tracking issue for something unstable. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. Z-profile-rustflags Nightly: profile-rustflags

Comments

@ehuss
Copy link
Contributor

ehuss commented Jan 7, 2022

Summary

Original issue: #7878
Implementation: #10217
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#profile-rustflags-option

The profile-rustflags feature allows setting RUSTFLAGS per-profile.

Unresolved Issues

  • How should things interact with rustdoc? If there is a flag that is required to correctly build a project, then cargo test may fail if they aren't passed to rustdoc. However, rustdoc doesn't accept all of rustc's flags. Adding a rustdocflags option seems to be adding more unwanted complexity, though.
  • This introduces potential backwards-compatibility hazards. For example, when Cargo adds new features or changes the way it invokes rustc, that can interfere with pre-existing flags. Is documenting this sufficient?
  • What should the behavior be with build scripts and proc-macros? Those have historically been a pain point with rustflags. Perhaps there should be a default build-override.rustflags = [].
  • Profile rustflags are not considered as part of TargetInfo, which impacts feature resolution and other things.
  • Are we ok with rustflags that can be set in Cargo.toml

Future Extensions

No response

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

@bstrie
Copy link
Contributor

bstrie commented Apr 29, 2022

I see something like this as necessary for the usability of RFC 3028 (artifact dependencies): #9096 . Historically Cargo has not supported passing arbitrary flags in profile overrides because many of them made no sense to pass to a dependency (e.g. -C linker). However, with binary artifacts, now literally all rustc flags become something that users will have valid reasons to want to specify. Currently this leads to lots of pain and workarounds with trying to use binary dependencies, either by hacking things up with Cargo config files (which don't work in all cases) or build scripts (which don't support all flags) or environment variables (which are difficult to set in all circumstances).

That doesn't mean that this feature is the only way to satisfy this need, but I think anything that does satisfy this need would have to be functionally equivalent to profile-rustflags.

@Qubasa
Copy link

Qubasa commented Sep 29, 2022

cargo-features = ["per-package-target", "profile-rustflags"] don't work together.
By this I mean the target does not get applied when profile-rustflags get used.

cargo-features = ["per-package-target", "profile-rustflags"]

[package]
name = "perf_kernel"
version = "0.1.0"
authors = ["Luis Hebendanz <luis.nixos@gmail.com>"]
edition = "2021"
forced-target = "x86_64-unknown-none"

[profile.dev]
rustflags = [ "-C", "link-args=--image-base=0x200000" ]

Will fail with gcc: error: unrecognized command-line option '--image-base=0x200000' even though target defines ld.lld as linker

@MauriceKayser
Copy link

MauriceKayser commented Jan 28, 2023

I can not get this to work with a no_std crate on Windows with the current nightly. Steps to reproduce the linker issue:

  1. Create a new binary project.
  2. Cargo.toml:
    [... default content ...]
    
    [profile.release]
    panic = "abort"
  3. src/main.rs:
    #![no_main]
    #![no_std]
    
    #[panic_handler]
    fn rust_begin_unwind(_: &core::panic::PanicInfo) -> ! { loop {} }
    
    #[no_mangle]
    fn WinMainCRTStartup() -> u32 { 0 }
  4. .cargo/config.toml:
    [target.'cfg(target_env = "msvc")']
    rustflags = [ "-C", "link-args=/Debug:None /SubSystem:Windows" ]
  5. Compile with cargo build --release --target x86_64-pc-windows-msvc -Z build-std=core -> it succeeds.
  6. Delete .cargo/config.toml.
  7. Cargo.toml:
    cargo-features = [ "profile-rustflags" ]
    
    [... previous content ...]
    
    rustflags = [ "-C", "link-args=/Debug:None /SubSystem:Windows" ]
  8. Compile with cargo build --release --target x86_64-pc-windows-msvc -Z build-std=core -> it succeeds.
  9. Delete the target build artifact directory to enforce a complete recompilation.
  10. Compile with cargo build --release --target x86_64-pc-windows-msvc -Z build-std=core -> it fails:
    Compiling compiler_builtins v0.1.85
    Compiling rustc-std-workspace-core v1.99.0
    
    error: linking with `link.exe` failed: exit code: 1120
    [... linker command arguments ...]
    note: msvcrt.lib(exe_winmain.obj) : error LNK2019: unresolved external symbol WinMain referenced in function "int __cdecl __scrt_common_main_seh(void)" (?__scrt_common_main_seh@@YAHXZ)
    build_script_build-[RandomHash].exe : fatal error LNK1120: 1 unresolved externals

It seems like it tries to link the things that should be excluded via no_std, like msvcrt and WinMain.
By the way, is it possible to use something like cfg(target_env = "msvc") with profile-rustflags?

@ehuss
Copy link
Contributor Author

ehuss commented Jan 28, 2023

As mentioned in the unresolved section, profile rustflags get applied to build scripts. You can override that with:

[profile.dev.build-override]
rustflags = []

Also, I might suggest using the #[windows_subsystem] attribute instead of setting it manually.

Conditional settings are not supported in profiles, you'll need to use separate profiles.

In the future, please file a new issue if you have a question or problem. See the "About tracking issues" section above.

@MauriceKayser
Copy link

Sorry, I will read more carefully next time and create a separate issue instead. I did not realize that my example contained a build script, thanks for the hint! The #![windows_subsystem = "windows"] seems to be ignored, if applied to the example - it will look for mainCRTStartup, the console subsystem main function, instead of WinMainCRTStartup, the windows subsystem main function.

@ehuss ehuss added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. labels Apr 25, 2023
@epage epage moved this to Unstable, baking in Cargo Roadmap Sep 5, 2023
@simbleau
Copy link

simbleau commented Dec 5, 2023

I am getting an issue where workspace rustflags don't work for sub-crates.

For example, - in the workspace Cargo.toml:

# TODO: Remove when WebGPU is no longer an unstable
[target.'cfg(target_arch="wasm32")'.profile.dev]
rustflags = ["--cfg", "web_sys_unstable_apis"]

Subcrates won't compile that depend on this:

error[E0599]: no method named `gpu` found for struct `WorkerNavigator` in the current scope
   --> /Users/simbleau/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-0.17.2/src/backend/web.rs:950:18
    |
947 | /             global
948 | |                 .unchecked_into::<web_sys::WorkerGlobalScope>()
949 | |                 .navigator()
950 | |                 .gpu()
    | |                 -^^^ method not found in `WorkerNavigator`
    | |_________________|

cargo build --verbose confirms the flags are not passed down correctly.

Related: #4897

@weihanglo
Copy link
Member

# TODO: Remove when WebGPU is no longer an unstable
[target.'cfg(target_arch="wasm32")'.profile.dev]
rustflags = ["--cfg", "web_sys_unstable_apis"]

This is not a valid key in Cargo.toml. You might get a warning like

warning: /path/to/pkg/Cargo.toml: unused manifest key: target.cfg(target_arch="wasm32").profile

@simbleau
Copy link

simbleau commented Dec 5, 2023

# TODO: Remove when WebGPU is no longer an unstable
[target.'cfg(target_arch="wasm32")'.profile.dev]
rustflags = ["--cfg", "web_sys_unstable_apis"]

This is not a valid key in Cargo.toml. You might get a warning like

warning: /path/to/pkg/Cargo.toml: unused manifest key: target.cfg(target_arch="wasm32").profile

Confused what you mean by it's not a key.

It's documented here: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#profile-rustflags-option

@intelfx
Copy link

intelfx commented Mar 20, 2024

Can we also have per-profile environment variables, for CFLAGS/CXXFLAGS? Before I make a corresponding issue, is this idea going to fly at all?

Rationale: RUSTFLAGS alone is not enough to have, for example, cross-language LTO on the release profile.

@weihanglo
Copy link
Member

Depends on how you use profile. For now we can leverage --config <alternative-config.toml> without waiting for such feature being implemented.

BTW I feel like this has been brought up, but couldn't find an issue of it.

intelfx added a commit to intelfx/bin that referenced this issue Apr 3, 2024
Wrapper for `cargo --profile=release` with a custom config, to make up
for absence of per-profile options (flags, env) in (stable) Cargo.
intelfx added a commit to intelfx/bin that referenced this issue Apr 3, 2024
Wrapper for `cargo --profile=release` with a custom config, to make up
for absence of per-profile options (flags, env) in (stable) Cargo.
intelfx added a commit to intelfx/dotfiles that referenced this issue Apr 3, 2024
Submodule bin 9be07f2..bc34b46:
  > wrappers: cargo-rel: workaround for rust-lang/cargo#10271 and co.
  > lib: bump
  > wrappers: komake: build an in-tree kernel module as an out-of-tree one
  > git/misc: git-delete-refs: tee delete commands on stderr
  > devel: unfuck.sh: cleanup
  > devel: replace unfuck-*.sh with unfuck.sh
  > misc: syncthing-curl: do not `curl -f`
intelfx added a commit to intelfx/dotfiles that referenced this issue Apr 7, 2024
Submodule bin 9be07f2..74e6e1c:
  > etc/easyrsa: add common easyrsa vars for intelfx.name PKIs
  > lib: bump
  > git/misc: git-edit: use `git add --intent-to-add` on all changed files
  > Merge branch 'master' of github.com:intelfx/bin
  > build-*-full: allow specifying SAN as a command option
  > easyrsa: init-pki: fix bogus unset when printing the new PKI status
  > easyrsa: init-pki: actually find vars.example outside of $EASYRSA_PKI
  > easyrsa: export(pkcs): only check ca.crt
  > tools: easyrsa: import easyrsa 3.1.7
  > misc: promote pkgbuild-* to misc
  > wrappers: lsblk-pt: `lsblk` with columns about partitions themselves
  > pass: get.bash: add pass(1) extension to retrieve specific fields in "key: value" format
  > util: disk-fstrim: always skip BIOS boot partitions (gdisk type EF02)
  > util: disk-fstrim: use lsblk instead of blkid
  > util: disk-fstrim: clean up variable usage in pattern contexts
  > util: disk-fstrim: clean up locals handling
  > wrappers: cargo-rel: workaround for rust-lang/cargo#10271 and co.
  > lib: bump
  > wrappers: komake: build an in-tree kernel module as an out-of-tree one
  > git/misc: git-delete-refs: tee delete commands on stderr
  > devel: unfuck.sh: cleanup
  > devel: replace unfuck-*.sh with unfuck.sh
  > misc: syncthing-curl: do not `curl -f`
@kamulos
Copy link

kamulos commented Apr 9, 2024

Depends on how you use profile. For now we can leverage --config <alternative-config.toml> without waiting for such feature being implemented.

BTW I feel like this has been brought up, but couldn't find an issue of it.

@weihanglo this sounds like a great solution, I just cant get it to work reasonably

let's say I have a default configuration that is used during development and in the dev profile. This should be a default and work without an extra command line which could be forgotten, because I want to put things like sanitizer support in there.

So this is my .cargo/config.toml:

[build]
target = "x86_64-unknown-linux-gnu"
rustflags = ["-Z", "sanitizer=address"]

[env]
BUILD_PROFILE = "debug"

Now in my build system I want to build the release build with a different configuration and the stable compiler. I create a new .cargo/config-release.toml with the following content:

[env]
BUILD_PROFILE = "release"

And use the command cargo +stable --config .cargo/config-release.toml to build it. Now it will still fall back to the .cargo/config.toml which will not work because there is no sanitizer in the stable compiler. Only once I delete the .cargo/config.toml, it will start using the config-release.toml.

How would you approach such a use case? I have a mixed C and Rust application, that needs different rustflags and environment variables for the debug and release builds.

I am really struggling with getting that right. Another really unfortunate thing with defining the rustflags in the config.toml, everytime I set the RUSTFLAGS env variable all the entries in the config.toml are silently thrown away. I just realized today, that I am setting RUSTFLAGS="${RUSTFLAGS} -D warnings" in the CI and by doing this am disabling all the flags I set in the config.

@weihanglo
Copy link
Member

@kamulos
if config-include supports optional config.toml, that could be an approach to manage multiple config files. See #7723.

@esemeniuc
Copy link

would love to see this make it to stable :)

@RalfJung
Copy link
Member

How does this interact with RUSTFLAGS? Do the flags get merged or does one take precedence?

@weihanglo
Copy link
Member

As of cargo 1.82.0-nightly (0d8d22f83 2024-08-08), the profile rustflags are passed to rustc invocations independently to other existing stable mechanism (build.rustflags, target.<triple>.rustflags, cargo rustc -- <flags>, RUSTFLAGS etc.), and has the lowest precedence.

cmd.args(&profile_rustflags);

@RalfJung
Copy link
Member

I am confused -- so both RUSTFLAGS and profile flags are passed? That means there is no precedence as the lists get merged? Saying "passed independently" and "lowest precedence" seems like a contradiction to me.

@weihanglo
Copy link
Member

By lowest precedence I meant they are passed earlier.

Cargo doesn't merge them. Cargo passes them to rustc verbatim. Flags from -Zprofile-rustflags are appended first. Then cargo rustc -- <args>. Then RUSTFLAGS. For example rustc <profile-rustflags> <cargo-rustc> <RUSTFLAGS>.

@RalfJung
Copy link
Member

RalfJung commented Aug 10, 2024 via email

@JohnstonJ
Copy link

What if I want to set rustflags per-profile and per-target?

e.g. say I want to use the mold linker, but only for Linux, and only for dev profile:

[target.x86_64-unknown-linux-gnu.profile.dev]    # or alternatively a cfg expression for the target
linker = "clang"
rustflags = ["-C", "link-arg=-fuse-ld=/path/to/mold"]

but I can't find a way to do this in the unstable feature documentation. Is it possible?

@weihanglo
Copy link
Member

@JohnstonJ No. It is another feature request. Per-target profile is tracking under #4897. See also previous comments #10271 (comment).

@BatmanAoD
Copy link
Member

It appears to me that this permits Cargo users to expose themselves to potential soundness issues to which they were previously immune. There's a discussion here about it, but the short version is that when the user has control over per-package Rust-flags, they can choose to compile some packages with panic=abort and others with panic=unwind. So far, I think that's still safe, because rustc itself will refuse to link the panic=unwind runtime if any crates in the dependency graph were compiled with panic=abort (unless the relevant part of RFC 1513 is no longer correct). But even if the panic=abort runtime is linked, it's still possible to run into UB by calling functions in other languages that unwind.

If my understanding is correct, there should be a prominent warning in the documentation for this feature that by changing the compilation flags for individual crates, the user loses some of the protections typically afforded to Cargo users.

Additionally, when this feature is stabilized, the "note" in the "Prohibited Linkage Scenarios" section of the reference (not merged yet) should be changed to acknowledge that this feature enables bypassing Cargo's protection against this particular UB:

Note: by default, Cargo compiles all crates in a dependency graph with the same panic runtime, and then links the final binary against the same panic runtime. This ensures that it is safe to have dependencies that specify a different panic runtime than the one ultimately used by your project, because the dependency crate's selected panic runtime will be ignored. However, it is possible to manually specify compile flags for individual packages using a [profile][cargo-profiles] that includes package.<name>.rustflags; this makes it possible to bypass any such protection Cargo might be able to provide.

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2024

Generally the expectation is that rustc must ensure that the crates it is linking are actually compatible for linking. So if this cargo feature can be used to cause unsoundness, that sounds like a soundness bug in rustc to me.

@BatmanAoD
Copy link
Member

BatmanAoD commented Oct 8, 2024

It's not just this feature, though: it's this feature, in conjunction with an FFI call to another language that then starts an unwind operation. rustc can't detect all the conditions required to cause UB here, which is why we created a lint, ffi_unwind_calls, to help users avoid it.

Edit: I think I was mistaken; rustc should simply refuse to linked in this case, not produce a binary with UB. rust-lang/rust#97235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-profiles Area: profiles A-rustflags Area: rustflags C-tracking-issue Category: A tracking issue for something unstable. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. Z-profile-rustflags Nightly: profile-rustflags
Projects
Status: Unstable, baking
Development

No branches or pull requests