Skip to content

Conversation

lambdageek
Copy link
Contributor

Adds Windows resources with the rust version information to rustc-main.exe and rustc_driver.dll

Invokes rc.exe directly, rather than using one of the crates from the ecosystem to avoid adding dependencies.

A new internal rustc_windows_rc crate has the common build script machinery for locating rc.exe and constructing the resource script

@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@lambdageek

This comment was marked as outdated.

@bjorn3
Copy link
Member

bjorn3 commented Aug 29, 2025

Why is the rustc version included in the product name? At most including the release channel would make sense to me (given that nightly genuinely behaves differently from stable by allowing unstable features), but the exaxt version is duplicated with the product version field.

@rust-log-analyzer

This comment has been minimized.

@lambdageek
Copy link
Contributor Author

Why is the rustc version included in the product name?

I don't have a good justification for the first iteration of this PR. Happy to update the names/descriptions to whatever makes the most sense

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@lambdageek
Copy link
Contributor Author

Updated the Product Description to be just "Rust Compiler" or "Rust Compiler (channel)" for non-stable

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Aug 30, 2025

This needs someone who actually have some clues about windows resources to review... I'll ask about a reviewer

@rustbot

This comment has been minimized.

@jieyouxu jieyouxu added the O-windows Operating system: Windows label Aug 30, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Aug 30, 2025

Adds Windows resources with the rust version information to rustc-main.exe and rustc_driver.dll

I'm not familiar with Windows resources. But can you say more on the motivation for this change?

EDIT: okay I found #t-compiler/windows > version resources on rustc.exe and rustc_driver.dll, but it's still not super obvious to me the motivation for the change.

@jieyouxu jieyouxu assigned jieyouxu and unassigned ChrisDenton Aug 30, 2025
@lambdageek
Copy link
Contributor Author

lambdageek commented Aug 30, 2025

In many ways this is a cosmetic change: as you can see in the screenshot in the comment above, Windows shows the version info in the file explorer when you right click on the .exe or .dll and look at the details

However this info is also used by some other tools on Windows such as debuggers or crash reporters when collecting diagnostic information.

For our internal builds of Rust at Microsoft having version info available would allow us to collect better automated crash reports from our users.
I could just add this only in our internal builds, but it seemed like it would be useful to upstream it.

@jieyouxu
Copy link
Member

Ok thanks for the clarification, that makes sense. I'll ask internally for another reviewer who has at least slightly more clues about this than I do.

@rustbot rustbot added this to the 1.91.0 milestone Sep 9, 2025
Copy link
Contributor

github-actions bot commented Sep 9, 2025

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing e9b6085 (parent) -> fefce3c (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard fefce3cecd63cebf2d7c9aa3dd90a84379fcfa1a --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-apple-various: 3117.2s -> 5295.3s (69.9%)
  2. dist-x86_64-apple: 5617.0s -> 9013.2s (60.5%)
  3. pr-check-1: 1732.5s -> 1359.0s (-21.6%)
  4. dist-various-1: 3738.0s -> 4319.2s (15.6%)
  5. aarch64-gnu-llvm-19-1: 3894.5s -> 3340.1s (-14.2%)
  6. i686-gnu-2: 6412.3s -> 5517.5s (-14.0%)
  7. aarch64-gnu-llvm-19-2: 2591.1s -> 2245.5s (-13.3%)
  8. x86_64-gnu-llvm-19-1: 3626.7s -> 3232.3s (-10.9%)
  9. x86_64-gnu-miri: 4931.9s -> 4410.0s (-10.6%)
  10. i686-gnu-1: 8272.1s -> 7423.4s (-10.3%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fefce3c): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.8% [3.8%, 3.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 466.867s -> 467.971s (0.24%)
Artifact size: 387.52 MiB -> 387.55 MiB (0.01%)

@nico
Copy link

nico commented Sep 11, 2025

Hardcoding rc.exe instead of allowing llvm-rc or some other res-writing crate prevents cross-building rustc-for-Windows on non-Windows hosts, no?

@ilovepi
Copy link
Contributor

ilovepi commented Sep 11, 2025

Hardcoding rc.exe instead of allowing llvm-rc or some other res-writing crate prevents cross-building rustc-for-Windows on non-Windows hosts, no?

Yeah this is also a problem for builds of Fuchsia's toolchains. Is there some way we can specify the path to the build directly? Maybe via the config.toml? Or an environment variable?

@bjorn3
Copy link
Member

bjorn3 commented Sep 12, 2025

Why is it a problem for Fuchsia? Are you trying to cross-compile from Fuchsia to Windows? rc.exe shouldn't be invoked if you are compiling for any non-Windows target.

@ilovepi
Copy link
Contributor

ilovepi commented Sep 12, 2025

Why is it a problem for Fuchsia? Are you trying to cross-compile from Fuchsia to Windows? rc.exe shouldn't be invoked if you are compiling for any non-Windows target.

No, we're building a Windows toolchain on Windows. We do also cross-compile from Linux, but our production builds are native Windows builds. Its a problem because we don't control the build of rc.exe, and we use all the llvm tools on windows to build Rust, Clang, and LLVM over their windows counterparts. Ideally the Windows SDK we use wouldn't even have the pieces we won't use, but we haven't gone that far w/ our SDK management yet.

FWIW, I don't think we'll be the only people that prefer these tools or want a way to specify where to look/find them directly.

@lambdageek
Copy link
Contributor Author

lambdageek commented Sep 12, 2025

@ilovepi so you'd want some way to specify a path to a resource compiler executable that basically accepts the same command line arguments as rc.exe? Something like llvm-rc. That makes sense

Update and you'd be using something like lld-link for linking? In particular something that has the same behavior as link.exe when passed a .res file as an input? This PR kind of relied on that, instead of explicitly invoking cvtres.exe to convert the .res file to an object file.

@ilovepi
Copy link
Contributor

ilovepi commented Sep 12, 2025

@ilovepi so you'd want some way to specify a path to a resource compiler executable that basically accepts the same command line arguments as rc.exe? Something like llvm-rc. That makes sense

Update and you'd be using something like lld-link for linking? In particular something that has the same behavior as link.exe when passed a .res file as an input? This PR kind of relied on that, instead of explicitly invoking cvtres.exe to convert the .res file to an object file.

yeah, we use all the llvm-* versions of the windows tools when possible, it isn't always, but we're trying to be hermetic and control all the inputs to our toolchain builds carefully to the extent we can. Anyway, those tools are intended to be drop in replacements for link.exe, rc.exe, etc. so I don't think stuff would break.

I'm pretty sure we just want some way to say: use the value from the config file/environment, otherwise fall back to this logic. so if you opt into setting it yourself it just uses those paths. If you didn't your approach seems totally reasonable.

@nico
Copy link

nico commented Sep 15, 2025

Any news here? Our (Chromium) builds of rustc (on Windows) are blocked on this, since we don't have MSVC tools on PATH.

erickt added a commit to erickt/rust that referenced this pull request Sep 17, 2025
In rust-lang#146018, it is now required to provide a resource compiler on windows
when compiling rust. This allows toolchain builders to explicitly
provide a path to an alternative, such as llvm-rc, instead of the one
that's provided by the Windows SDK.
erickt added a commit to erickt/rust that referenced this pull request Sep 17, 2025
In rust-lang#146018, it is now required to provide a resource compiler on windows
when compiling rust. This allows toolchain builders to explicitly
provide a path to an alternative, such as llvm-rc, instead of the one
that's provided by the Windows SDK.
@zeroomega
Copy link
Contributor

This PR also broke our (Fuchsia's) rustc build on Windows. As it failed to locate the rc.exe. Even though rc.exe was in current system's search path. We spent a bit of time try to instrument this PR and to see what went wrong. And it looks like in your patch https://github.com/rust-lang/rust/pull/146018/files#diff-8a40df8ee7be3d238589677ae282637037040cceca3dc167c3e234db981245cfR143-R150

The env map from the msvc_linker could be empty even though the linker.exe was found, the returned msvc_linker value was :

Tool { path: "C:\\Program Files (x86)\\Microsoft Visual Studio\\2022\\BuildTools\\VC\\Tools\\MSVC\\14.38.33130\\bin\\HostX64\\x64\\link.exe", cc_wrapper_path: None, cc_wrapper_args: [], args: [], env: [], family: Msvc { clang_cl: false }, cuda: false, removed_args: [], has_internal_target_arg: false }

It looks like the find_msvc_environment function from https://github.com/rust-lang/cc-rs/blob/ac2192ced40bc82d69dd500bd561aa65c186610e/find-msvc-tools/src/find_tools.rs#L464 returned a Tool object with an empty environment map and your code

let msvc_linker = windows_registry::find_tool(arch_or_target, "link.exe")?;
let path = &msvc_linker.env().iter().find(|(k, _)| k == "PATH")?.1;

just assume the "PATH" is always exist in the returned mvsc_linker variable, when MSVC build tool is installed, which failed in our case.

Ideally, the search logic should fallback to search "rc.exe" in the current process's search path at least. Which in our case, it is always there, along with the "cl.exe" and the "linker.exe"

The build environment was set up through the "x64 Native Tools Command Prompt for VS 2022" tool, which is the one of the official way to setup msvc build environment for C++ development.

I would like to request a revert of this PR, since it didn't consider a common case of setting up the build environment on Windows and it affects multiple down stream users now.

@wesleywiser
Copy link
Member

wesleywiser commented Sep 17, 2025

@zeroomega It seems pretty clear that we should revert that PR, at least on beta. Could you open a bug report about this issue so we can track it and ensure it's considered for a beta revert at the appropriate compiler team triage meeting?

@lambdageek
Copy link
Contributor Author

lambdageek commented Sep 17, 2025

Attempted fix for master #146689

Revert for beta #146687

@zeroomega
Copy link
Contributor

zeroomega commented Sep 17, 2025

Filed #146693 . Please let me know if you need more info on how to reproduce it.

fmease added a commit to fmease/rust that referenced this pull request Sep 18, 2025
Allow windows resource compiler to be overridden

In rust-lang#146018, it is now required to provide a resource compiler on windows when compiling rust. This allows toolchain builders to explicitly provide a path to an alternative, such as llvm-rc, instead of the one that's provided by the Windows SDK.

cc `@lambdageek`
bors added a commit that referenced this pull request Sep 19, 2025
[beta] Revert "compiler: Add Windows resources to rustc-main and rustc_driver"

This reverts #146018 due to #146693
jhpratt added a commit to jhpratt/rust that referenced this pull request Sep 19, 2025
Allow windows resource compiler to be overridden

In rust-lang#146018, it is now required to provide a resource compiler on windows when compiling rust. This allows toolchain builders to explicitly provide a path to an alternative, such as llvm-rc, instead of the one that's provided by the Windows SDK.

cc `@lambdageek`
bors added a commit that referenced this pull request Sep 19, 2025
[beta] Revert "compiler: Add Windows resources to rustc-main and rustc_driver"

This reverts #146018 due to #146693
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 19, 2025
Allow windows resource compiler to be overridden

In rust-lang#146018, it is now required to provide a resource compiler on windows when compiling rust. This allows toolchain builders to explicitly provide a path to an alternative, such as llvm-rc, instead of the one that's provided by the Windows SDK.

cc ``@lambdageek``
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 19, 2025
Allow windows resource compiler to be overridden

In rust-lang#146018, it is now required to provide a resource compiler on windows when compiling rust. This allows toolchain builders to explicitly provide a path to an alternative, such as llvm-rc, instead of the one that's provided by the Windows SDK.

cc ```@lambdageek```
rust-timer added a commit that referenced this pull request Sep 19, 2025
Rollup merge of #146663 - erickt:win, r=wesleywiser

Allow windows resource compiler to be overridden

In #146018, it is now required to provide a resource compiler on windows when compiling rust. This allows toolchain builders to explicitly provide a path to an alternative, such as llvm-rc, instead of the one that's provided by the Windows SDK.

cc ```@lambdageek```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. merged-by-bors This PR was explicitly merged by bors. O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.