Fix rustc_tools_util's version.host_compiler release channel, expose the rustc version, and add tests
#14123
Conversation
… version, debug print, and test.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Centri3 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
|
FWIW, I would also happily replace host_compiler = "stable" | "nightly" | "beta" | undefined; with the verbose version string, e.g. "rustc 1.84.0 (9fc6b4312 2025-01-07)" The former is trivial to derive. Merely didn't do it in case someone depends on the current behavior, though unlikely since it was broken. Maybe one could just release a 0.5.0? |
rustc_tools_util/src/lib.rs
Outdated
| println!("cargo:rustc-env=RUSTC_RELEASE_CHANNEL={}", $crate::get_channel()); | ||
| let compiler_version = $crate::get_compiler_version(); | ||
| println!( | ||
| "cargo:rustc-env=RUSTC_VERSION={}", |
There was a problem hiding this comment.
What's the motivation for adding this? Where is it supposed to be used? And: Should it really be the full output of rustc -V and not just the actual version?
There was a problem hiding this comment.
Let me first apologize that I jugged a fix and a feature together.
What's the motivation for adding this?
I first fixed the existing channel metadata but came to wonder the same thing: what's the motivation? The channel [stable|nightly|beta] doesn't seem particularly useful (and it likes like no one was relying on it). My personal motivation is to bake metadata into binary about from what sources it was built (i.e. commit hash) and what compiler. The release channel is not sufficient for the latter.
And: Should it really be the full output of rustc -V and not just the actual version?
Happy to discuss the details. But I'm also thinking that it would be fine to push some of the post-processing from build time to runtime. This is related to the comment I made about just removing RUSTC_RELEASE_CHANNEL and extracting it from RUSTC_VERSION at runtime instead.
No strong opinions here, happy with anything the unambigiously identifies the compiler version.
There was a problem hiding this comment.
Removing the RELEASE_CHANNEL I would avoid for backwards compat reasons, in case there is someone out there relying on that.
I was going back and forth about the full string vs just the version for RUSTC_VERSION. I still don't know what would be better. I'm leaning towards just the version. In case the hash and date are also necessary in the future, we can add them as extra vars.
Maybe it is easier to parse the version out of rustc -vV instead of just -V.
There was a problem hiding this comment.
Happy to make the change, just thinking about this a bit more, isn't the version string meaningless for nightly? I.e it will always be: 1.86.0-nightly for all versions of 1.86 nightly.
There was a problem hiding this comment.
Yeah it is. Should we just not add the RUSTC_VERSION, as IIUC there is currently not really a use case for it? Or is this PR your "feature request" for this?
There was a problem hiding this comment.
Or is this PR your "feature request" for this?
That's what I meant in the beginning with:
Let me first apologize that I jugged a fix and a feature together.
I removed the version for now, leaving on the the channel fix. However I would really like to get the compiler version (or versions) in (in a follow-up), since the release channel is pretty meaningless (as witnessed by being broken and nobody noticing :))
There was a problem hiding this comment.
Don't get me wrong. I don't have anything against adding this in the same PR. I'd even rather do that than bump the version twice.
My issue is that I don't know how one would intend to make use of RUSTC_VERSION. Therefore, I don't know how best to add this. It seems like you have a use case in a personal project for this feature? If so, this could help inform how best we can add it.
There was a problem hiding this comment.
It seems like you have a use case in a personal project for this feature?
Happy to share. It's nothing too fancy. I have a database thingy and would like to share build metadata via its admin dashboard:
There was a problem hiding this comment.
Should we just move forward with just fixing the existing functionality?
…ing on the release channel fix for now.
|
r? flip1995 |
changelog: Fix rustc_tools_util's
version.host_compilerrelease channel, expose the rustc version, and add tests.Previously the host_compiler would be set to "nighly" on the stable channel. Generally, the field felt a bit neglected neither being printed not tested.