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

Add Windows application manifest to rustc-main #96737

Merged
merged 1 commit into from
Jun 2, 2022
Merged

Add Windows application manifest to rustc-main #96737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 28 additions & 0 deletions compiler/rustc/Windows Manifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<!--
This is a Windows application manifest file.
See: https://docs.microsoft.com/en-us/windows/win32/sbscs/application-manifests
-->
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0" xmlns:asmv3="urn:schemas-microsoft-com:asm.v3">
<!-- Versions rustc supports as compiler hosts -->
<compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
<application>
<!-- Windows 7 --><supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/>
<!-- Windows 8 --><supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/>
<!-- Windows 8.1 --><supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/>
<!-- Windows 10 and 11 --><supportedOS Id="{8e0f7a12-bfb3-4fe8-b9a5-48fd50a15a9a}"/>
</application>
</compatibility>
<!-- Use UTF-8 code page -->
<asmv3:application>
<asmv3:windowsSettings xmlns="http://schemas.microsoft.com/SMI/2019/WindowsSettings">
<activeCodePage>UTF-8</activeCodePage>
Copy link
Member

Choose a reason for hiding this comment

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

This only affects A suffixed Windows API usage, which Rust doesn't use anywhere. Rustc may have C/C++ dependencies which do however. The best way to be certain about this is to check whether there are any conversions between unicode and the narrow system encoding. Most like we're completely in the clear and this change would only improve things.

</asmv3:windowsSettings>
</asmv3:application>
<!-- Remove (most) legacy path limits -->
<asmv3:application>
<asmv3:windowsSettings xmlns:ws2="http://schemas.microsoft.com/SMI/2016/WindowsSettings">
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking this PR, but it would be worth testing if setting heapType provides any perf benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that would be interesting! But yes, I would prefer that to be a separate PR because of the perf differences.

Copy link
Contributor

@klensy klensy Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed, that would be interesting! But yes, I would prefer that to be a separate PR because of the perf differences.

But rust infra only have linux perf runner and not windows, so how to see this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it can be run locally or with a try build if you edit the configuration also. I'm not sure though.

In any case, I think that would be a problem for another PR rather than this one.

<ws2:longPathAware>true</ws2:longPathAware>
Copy link
Member

Choose a reason for hiding this comment

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

While this only takes effect if the system has long paths enabled on it (it is disabled by default), there is no downside to this so long as rustc has no issues with handling long paths (which Rust itself is absolutely fine with and the only concern would be specific C/C++ dependencies we have like LLVM but I'm pretty sure that's also fine).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this potentially change the way that rustc emits paths in error messages (i.e., using the \\?\ prefixed syntax)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should not change anything there. It allows C:\... paths to be used beyond the 260 limit without the \\?\ prefix but does not otherwise alter how rustc sees paths.

</asmv3:windowsSettings>
</asmv3:application>
</assembly>
24 changes: 24 additions & 0 deletions compiler/rustc/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
use std::env;

fn main() {
let target_os = env::var("CARGO_CFG_TARGET_OS");
let target_env = env::var("CARGO_CFG_TARGET_ENV");
if Ok("windows") == target_os.as_deref() && Ok("msvc") == target_env.as_deref() {
set_windows_exe_options();
}
}

// Add a manifest file to rustc.exe.
fn set_windows_exe_options() {
static WINDOWS_MANIFEST_FILE: &str = "Windows Manifest.xml";

let mut manifest = env::current_dir().unwrap();
manifest.push(WINDOWS_MANIFEST_FILE);

println!("cargo:rerun-if-changed={}", WINDOWS_MANIFEST_FILE);
// Embed the Windows application manifest file.
println!("cargo:rustc-link-arg-bin=rustc-main=/MANIFEST:EMBED");
println!("cargo:rustc-link-arg-bin=rustc-main=/MANIFESTINPUT:{}", manifest.to_str().unwrap());
// Turn linker warnings into errors.
println!("cargo:rustc-link-arg-bin=rustc-main=/WX");
Copy link
Member

@retep998 retep998 May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest, I wish we enabled this more often. Rust currently hides all output from the linker if it succeeds so linker warnings are completely not noticed, even if they may indicate something is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye, My specific motivation here was to catch manifest related warnings that are actually important. But I kinda think it's something that should be on by default for most projects unless they have a specific need to disable it (e.g. legacy code, etc).

}