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

build.rs should not detect nightly feature #323

Closed
loynoir opened this issue Oct 24, 2023 · 11 comments
Closed

build.rs should not detect nightly feature #323

loynoir opened this issue Oct 24, 2023 · 11 comments

Comments

@loynoir
Copy link

loynoir commented Oct 24, 2023

quote

rust-lang/cargo#12872 (comment)

Personally, I think auto-detecting nightly features is a mistake and instead feature flags should be used to enable them explicitly.

actual

build.rs detect nightly feature.

anyhow/build.rs

Lines 17 to 18 in 05e4132

const PROBE: &str = r#"
#![feature(error_generic_member_access)]

expected

build.rs should not detect nightly feature.

@dtolnay
Copy link
Owner

dtolnay commented Oct 24, 2023

I think this is all right as currently implemented. There have been bugs in rust-analyzer and elsewhere related to build scripts in the past, so that is the most likely thing leading to the "#![feature] may not be used on the stable release channel" that you quoted. But if you or the rust-analyzer folks are able to identify anything that our build script implementation is doing incorrectly I am open to PRs to fix that.

@dtolnay dtolnay closed this as completed Oct 24, 2023
@loynoir
Copy link
Author

loynoir commented Oct 25, 2023

@dtolnay

But if you or the rust-analyzer folks are able to identify anything that our build script implementation is doing incorrectly I am open to PRs to fix that.

epage at rust-lang/cargo

Personally, I think auto-detecting nightly features is a mistake and instead feature flags should be used to enable them explicitly.

I think he means something like this.

anyhow = { version = "xxx", features = ["nightly"] }

@dtolnay
Copy link
Owner

dtolnay commented Oct 25, 2023

The thing is, I don't agree. If something about feature detection is causing "#![feature] may not be used on the stable release channel" then that is a bug which can be fixed.

@epage
Copy link

epage commented Oct 25, 2023

Could you help me understand what is the design principle for why #[feature] detection is preferred over a nightly feature?

The thing is, I don't agree. If something about feature detection is causing "#![feature] may not be used on the stable release channel" then that is a bug which can be fixed.

The problem with that approach is that the bug then makes packages unusable on nightly unless they update their dependency for unstable features they never accepted the risks for. I'm still finding packages of mine with old proc_macro2. Besides checking verifying nightly itself isn't broken (after the whole breakage of rlua), I use nightly for miri.

This also comes with a build time cost that everyone is paying. Once an MSRV is raised high enough to not need version detection, then it shouldn't be needed. Instead, when dealing with nightly features, that is more of a never ending treadmill.

@dtolnay
Copy link
Owner

dtolnay commented Oct 26, 2023

The design principle is that users should get good errors. (1) The user should not need to make their crate incompatible with stable Rust in order to get good errors while working on it. (2) There should be O(1) work for n users to benefit from good errors; not O(n) work.

I don't think proc-macro2 is comparable with anyhow's build script. They use different approaches with different failure modes. I intend to reconcile them as soon as there is a clear good one, but for now it has been blocked by rust-analyzer and cargo issues.

The build time cost is overstated.

@epage
Copy link

epage commented Oct 26, 2023

The design principle is that users should get good errors. (1) The user should not need to make their crate incompatible with stable Rust in order to get good errors while working on it. (2) There should be O(1) work for n users to benefit from good errors; not O(n) work.

How prevalent are people specifically wanting these nightly improvements in a transient state? That feels like a corner of a corner case (stable users who know about this and specifically rebuild everything in nightly to debug something).

Assuming this is needed, it sounds like your concern is the cost of those users toggling this to get the benefit. To be clear, for people directly depending on it, that is cargo run -F anyhow/nightly and for indirect users, it would be adding a direct dependency so they can do the same thing. They might even add their own nightly feature to enable nightly features across the packages they use. In that case, they could be helped out if this was a --cfg or maybe my proposed concept of globals.

The build time cost is overstated.

At least for a common enough case (derive CLI parser for a "cargo script" using proc_macro2 though hadn't thrown anyhow into the mix), I just ran --timings and the build script was in the critical path, taking up 6% of the total build time. The compiler people are happy when they get 1-2% wins, so an instant 6% win is big. And this is just the overhead that --timings shows. I want to dig into general cargo overhead at some point since that tends to get glossed over when we collect numbers that I'm worry we have blindspots there (though I suspect its actually minuscule).

@dtolnay
Copy link
Owner

dtolnay commented Oct 26, 2023

How prevalent are people specifically wanting these nightly improvements in a transient state? That feels like a corner of a corner case (stable users who know about this and specifically rebuild everything in nightly to debug something).

I think counting people who want a specific improvement is not a good way to evaluate whether it's worthwhile. Users transparently benefit from better errors without noticing, and would have a worse experience otherwise. Asking them to perpetually think about whether they want good errors or not in a particular development session, in itself, would make the library worse. Feature detection produces errors that are as good as supported by the user's chosen toolchain.

The improvements are not a corner of a corner case, in my experience. For example proc_macro::Span::join is extremely significant to proc macros producing good errors. A double digit percentage of Rust developers develop on nightly and benefit from this transparently for macros that use syn.

@dzmitry-lahoda
Copy link

dzmitry-lahoda commented Jan 9, 2024

i have issue with current way of detection work. i build in sandbox, nix. it prevents randomly creating folders out of predefined location. and does not expect build.rs run rustc. so my builds failing.

https://github.com/dtolnay/anyhow/blob/master/build.rs#L91C21-L91C32

I will try disable via ENV var, so usually nightly can be disabled by other means of explicit configuration of were code is in. current feature name and setup seems not the way i can get benefits of it autoenabled. so its clear my prs are welcomed ;) i just do not use anyhow a lot(it was dep of dep).

@dzmitry-lahoda
Copy link

dzmitry-lahoda commented Jan 9, 2024

if I compile with detection disabled using usual way doing things (not nix),

I randomly get (may be 50% of time):

bash-5.2$ RUSTC_STAGE=1 cargo build --target wasm32-unknown-unknown --package common  --no-default-features --verbose

Fresh thiserror-impl v1.0.43
error: failed to run custom build command for `anyhow v1.0.72`

Caused by:
  could not execute process `/home/dz/github.com/ComposableFi/composable/code/target/debug/build/anyhow-37fe4b7d03a584fa/build-script-build` (never executed)

Caused by:
  No such file or directory (os error 2)

@spectria-limina
Copy link

As an end user, I do not want crates automatically opting into unstable features when nightly is detected. This is absolutely a security and build integrity concern. It absolutely needs to be opt in, especially when it is for minor functionality such as error messages.

@RalfJung
Copy link
Contributor

RalfJung commented Mar 26, 2024

Crates just automatically using nightly features fundamentally breaks the opt-in nature of rustc nightly. That's a social contract that Rust nightly feature development relies on, and like all social contracts it is fuzzy and it can tolerate some amount of stretching, but it's also dangerous to try and stretch it so far. The recent ahash debacle shows what can happen. Now, anyhow uses a more careful build probe than ahash, but it is very hard to be sure that "if the build probe compiles then my entire crate will definitely compile", so there's still a significant risk of causing wide-spread build failures.

Furthermore, build probes are extremely fragile, and (in the full generality of cargo build situations) not currently possible to implement properly (due to rust-lang/cargo#5755). I don't even know how many hours I spent debugging build probe issues, including in anyhow (see e.g. #249, #320). Miri and rustc bootstraping tend to hit corner cases that are not covered by regular usage. These were problems with the build probes, not with RA or cargo. So it's just not correct to claim that the issues were primarily with other tools.

Slightly nicer backtraces for nightly users only just doesn't seem worth that risk, nor the cost. (In particular since a significant part of the cost is carried by people that never had any say in the decision to add this.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants