Add a cargo feature to automatically detect usage of a nightly compiler.#5
Add a cargo feature to automatically detect usage of a nightly compiler.#5Alextopher wants to merge 2 commits intoGnomedDev:masterfrom
Conversation
|
I am split on this change, as I very much dislike nightly detection as it weakens the stability of Rust just for using an experimental toolchain, often without consent. However, this might not be my decision to make, and this feature would take it out of my hands. I'll discuss this with others. |
|
I agree it's an uncomfortable decision. I'm not much of a fan of build scripts in general, particularly because of the potential supply chain risk they pose. This was not the first solution I tried but I think there's a fundamental limitation to rust build chains here. I think there is a need for a fully compatible alternative to Thank you for your consideration! I look forward to the future where the macro diagnostics feature is stabilized and released 😃 |
build.rs
Outdated
| .expect("Failed to get rustc version") | ||
| .stdout; | ||
|
|
||
| let rustc_version = String::from_utf8(rustc_version).expect("Failed to parse rustc version"); | ||
| if rustc_version.contains("nightly") { | ||
| println!("cargo:rustc-cfg=detected_nightly"); | ||
| } |
There was a problem hiding this comment.
I don't like the idea of a build failing because an error diagnostics crate cannot detect if it can use nicer diagnostics. Can we get rid of the panics?
There was a problem hiding this comment.
Thinking about it some more this build script is incorrect. It's plausible rustc is not the name of the invoked compiler. I'm not sure what I was thinking, $RUSTC is needed.
There was a problem hiding this comment.
I think I may disagree with removing the panics.
$RUSTCorrustcdoesn't exist / there's a spurious OS error
Rather than build something non-deterministically we should "fail fast". If you want nightly detection you should get nightly detection.
rustc --versionchanges
Either it's no longer UTF-8 (not in my rust!) or "nightly" no longer appears in nightly toolchains. That seems very unlikely and would break crates like rustc_version
There was a problem hiding this comment.
Reminder: This is not the user's choice, this is a library's choice. If this crate is 5 levels deep in subdeps and it's nightly detection fails in an otherwise passing build, that is a build failure due to a crate wanting to display non-existent errors nicely and requires fixing. That's silly.
I would be okay if you set a config for "failed to nightly detect" and printed a warning alongside existing diagnostics, but that's as far as I am happy going with nightly detection failures.
There was a problem hiding this comment.
If you want nightly detection you should get nightly detection.
Sorry, maybe this wasn't the best way to make my point. Can we take a step back? My wonder is what are the specific failure modes you are concerned about.
There was a problem hiding this comment.
In the sentence "if you want nightly detection", you are talking about a "you". In this case, "you" is not the developer of the binary crate who is affected by build failures, it's the developer of the library depending on proc-macro-error2, and this mismatch is why I am generally against nightly detection in general.
I am not worried about any specific error conditions, I am concerned that nightly detection may lead to an otherwise passing build failing, which is silly for a feature to display nicer error messages.
Alextopher
force-pushed
the
detect-nightly
branch
from
bec8429 to
b34d561
Compare
| nightly = [] | ||
| detect-nightly = [] |
There was a problem hiding this comment.
Is there a good reason not to simplify things by getting rid of both these feature flags and always using the build script?
There is a reason: to ensure compatibility with future nightly rustc using an incompatible reporting interface. This seems a moot point since (a) a fix can likely be published quickly as a patch release (no one supports old nightlies thus there will not be any supported working configuration to break), (b) stable will still work and (c) it shouldn't be that hard to work around using [patch.crates-io.proc-macro-error-2].
Otherwise...
- The existing
nightlyfeature needs to be forwarded from whatever proc-macro lib uses this crate, and that lib needs to tell its users to add their own feature forwardingnightlyand to use this when developing, as well as to add a CI test which uses this feature and denies warnings. This path is theoretically workable but has a lot of overhead. - This
detect-nightlyfeature needs to be forwarded from whatever proc-macro lib uses this crate, and that lib needs to tell its users to enable it (either permanently or with their own feature flag used in testing as above). Since even the crate using the proc-macro crate which uses this crate is probably itself a library, it still is likely in the same situation as this crate about what it may assume. - The proc-macro built from this crate may permanently enable
detect-nightlysince this crate doesn't.
Further, the crate docs still seem to imply that warnings happen on nightly automatically.
My take: both these feature flags should be removed and detection should be automatic.
There was a problem hiding this comment.
Further, the crate docs still seem to imply that warnings happen on nightly automatically.
That's because this crate is forked from proc-macro-error which had build scripts for nightly detection. The authors of this crate removed the scripts on the path to syn2.
I think the authors of this repo should list out their goals with the project. Namely where does compatibility with pme1 stack up to our own thoughts on build systems? proc-macro-error used a build script and we're only here because folks are still using it.
There was a problem hiding this comment.
My take: both these feature flags should be removed and detection should be automatic.
I agree.
There was a problem hiding this comment.
I'm here because (1) this was an easy transition and (2) the alternatives did not support reporting-as-a-side-effect (emit_*!).
So, yes. And I'm not so happy to see that warnings are no longer being reported (without extra set up on my end that I didn't know I had to do).
The best option would be rustc stabilising rust-lang/rust#54140 or equivalent. The next best option is something like proc-macro-error.
There was a problem hiding this comment.
Also on this topic: SergioBenitez/version_check#23
Clearly some people have good reasons for not wanting any automatic opt-in to nightly features. This goes as far as affecting development of the compiler, since it complicates testing whether a nightly compiler has regressions over stable. To me this is a good argument not to automatically opt in to nightly features.
Are they any greater a risk than proc-macros which also run at compile-time? Then there are tests and examples which often get run on dev machines, though at least IDEs aren't likely to automatically run these. |
3 participants
I wanted to contribute removing
proc-macro-errorfrom yew. However,proc-macro-error2isn't a drop in replacement for them because of the removal of build-time nightly compiler detection.This PR adds a feature
detect-nightlywhich uses a build script to detect if a nightly compiler is being used. With this detect-nightly flagproc-macro-error2seems to be 1-1 withproc-macro-errorwith respect to the nightly proc_macro_diagnostics usage.Tradeoffs
I did my best to minimize the cost of this change. Build scripts always incur a cost and sadly after all my research I've found it's impossible to detect usage of nightly compiler without a build script.
Without increasing project complexity these are my results:
BEFORE
cargo +nightly build --release --timings -j1 --features=nightlyAFTER
cargo +nightly build --release --timings -j1 --features=detect-nightlyWe could abstract usage of nightly features into it's own crate that optionally compiles based on the
detect-nightly/nightlyflag. In that case stable users would still have 12 units, nightly users would have 15.