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.

Sign up for GitHub

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

Jump to bottom

RFC: Add target configuration #2991

Closed
wants to merge 2 commits into from
Closed

RFC: Add target configuration #2991

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27964b4
RFC: Add `target` configuration
nvzqz Sep 27, 2020
63fd993
Add RFC PR to `cfg-target`
nvzqz Sep 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions text/0000-cfg-target.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
- Feature Name: `cfg-target`
- Start Date: 2020-09-27
- RFC PR: [rust-lang/rfcs#2991](https://github.com/rust-lang/rfcs/pull/2991)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary
[summary]: #summary

This proposes a new `cfg`: `target`, which matches the entire target triple
string (e.g. `arm-unknown-linux-gnueabihf`). This also adds a `CARGO_CFG_TARGET`
environment variable for parity with other `CARGO_CFG_*` variables.

# Motivation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the motivation section provides enough of a compelling argument for why this is desirable. Fleshing this out with specific examples of existing crates that use workarounds to achieve this and why they do so would be useful.

[motivation]: #motivation

To `#[cfg]` against a specific target, a `build.rs` script is required to emit a
custom `cfg` based on the `TARGET` environment variable. Adding a build script
increases compile time and makes a crate incompatible with certain build
systems.

Otherwise, all available components would need to be specified separately:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely remember that the target isn't necessarily the concatenation of the various target_{foo} parts. That said, I'm having trouble finding the part of the docs that says this...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x86_64-fuchsia is an example (it doesn't concatenate unknowns into the result).

cc @petrhosek @tmandry

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clang/LLVM treats empty components, none and unknown as equivalent, so for example x86_64-fuchsia, x86_64-unknown-fuchsia and x86_64-none-fuchsia are all considered equivalent and internally normalized to x86_64-unknown-fuchsia.

We prefer the shortest spelling for convenience, but if Rust prefers always using normalized triples, we could switch to using x86_64-unknown-fuchsia, I'd be fine with that. This wasn't possible in the past when LLVM normalized empty components inconsistently, but that issue has since been resolved.

`target_arch`, `target_vendor`, `target_os`, and `target_env`. This can be very
cumbersome. Note that the target ABI cannot currently be `#[cfg]`-ed against, so
a `build.rs` is still necessary to match all target components.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

This would act like existing `target_*` configurations but match against all
components (except `target_feature`).

```rust
#[cfg(target = "x86_64-apple-ios-macabi")]
mod mac_catalyst;
```

This includes `#[cfg_attr(target = "...", attr)]`.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

`target` is a key-value option set once with the target's Rust triple.

Example values:

- `"aarch64-apple-darwin"`
- `"arm-unknown-linux-gnueabihf"`
- `"x86_64-apple-ios-macabi"`
- `"x86_64-pc-windows-gnu"`
- `"x86_64-pc-windows-msvc"`
- `"x86_64-unknown-linux-gnu"`

# Drawbacks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another drawback might be that technically this could be a breaking change with anybody that conditionally adds a --cfg target=... cfg either via RUSTFLAGS or cargo:rustc-cfg=target=... from a build script.

This goes for your target_abi PR also.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so if a person's build.rs had lines like say

  let target = env::var("TARGET").expect("Couldn't read `TARGET`");
  println!("cargo:rustc-env=TARGET={}", target);

Then technically they'd hit a small break ;P

Copy link
Member

@thomcc thomcc Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that should be fine. The code would have to be:

let target = env::var("TARGET").expect("Couldn't read `TARGET`");
println!("cargo:rustc-cfg=target={}", target);

in order to have a break... Actually, this would define the target identically to what it was before, so they'd have to define it as something different (unless their definition would take priority?), or only in some cases, I guess.

This is a problem adding any new predefined cfg faces, and stagnating the set of cfgs feels pretty undesirable to me, so I don't know if it's a case worth caring about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string is equal to the TARGET environment var anyway, so target would not change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I meant by:

Actually, this would define the target identically to what it was before, so they'd have to define it as something different (unless their definition would take priority?), or only in some cases, I guess.

It might be a breaking change for:

let target = env::var("CARGO_CFG_TARGET_ARCH").expect("Couldn't read `CARGO_CFG_TARGET_ARCH`");
println!("cargo:rustc-cfg=target={}", target);

or

if some_condition {
    let target = env::var("TARGET").expect("Couldn't read `TARGET`");
    println!("cargo:rustc-cfg=target={}", target);
}

But these are pretty obscure cases...

[drawbacks]: #drawbacks

- Configuring against specific targets can be overly strict and could make
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the biggest drawback in my experience. People will often reach for the most accessible way to target a platform even if it's the wrong one. We saw this a lot in the Firefox codebase where people would use the #ifdef for "using the GTK widget implementation" when they really meant "this is Linux" or other similar things.

certain `#[cfg]`s miss similar configurations with small changes.

For example: `aarch64-unknown-none` does not match
`aarch64-unknown-none-softfloat`, yet one would likely want to include ABI
variants. The same concern applies to the target vendor.

A potential solution would be to allow glob matching (e.g.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How feasible would it be to allow specifying individual components of the target triple string by keyword inside a target cfg, something like:

#[cfg(target(arch="aarch64", os="none"))]

which would be equivalent to:

#[cfg(all(target_arch="aarch64", target_os="none"))]

?

I definitely agree with the assertion that "matching the entire target with the current set of cfg options is extremely verbose", but I'm not convinced that "allow matching the entire target string directly" is the right way to address that.

`aarch64-unknown-none*`), but that is not within the scope of this proposal
because it is not currently used in other `#[cfg]`s.

- The `CARGO_CFG_TARGET` environment variable is redundant with the existing
`TARGET`. However, including it would be consistent with other `CARGO_CFG_*`
variables.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

We can keep the existing work-around of checking the `TARGET` environment
variable in a `build.rs` script. However, that increases compile time and makes
a crate incompatible with certain build systems.

# Prior art
[prior-art]: #prior-art

- [Target component configurations](https://doc.rust-lang.org/reference/conditional-compilation.html#set-configuration-options):
`target_arch`, `target_vendor`, `target_os`, and `target_env`.

- `TARGET` and `CARGO_CFG_TARGET_*`
[environment variables for `build.rs`](https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts).

# Unresolved questions
[unresolved-questions]: #unresolved-questions

- How do we ensure a project does not miss configurations similar to the ones
being `#[cfg]`-ed against with this feature? Perhaps this should be added as a
Clippy lint that's off by default.

# Future possibilities
[future-possibilities]: #future-possibilities

This would enable `#[cfg]`-ing against a specific target ABI (e.g. `macabi`,
`eabihf`). However, that is not the motivation for this proposal and should be
handled separately.