-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
pkg: embed windows manifest to support long file paths if enabled #2049
pkg: embed windows manifest to support long file paths if enabled #2049
Conversation
this is also what https://github.com/nabijaczleweli/cargo-update is using |
Works fine for me with the registry key |
use app::{RGArg, RGArgKind}; | ||
extern crate embed_resource; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is no longer needed since Rust Edition 2018.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this should be removed.
@@ -48,6 +48,9 @@ fn main() { | |||
if let Some(rev) = git_revision_hash() { | |||
println!("cargo:rustc-env=RIPGREP_BUILD_GIT_HASH={}", rev); | |||
} | |||
|
|||
// embed windows application manifest file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see browsing through the code base, comments are written as sentences, i.e. they start with a capital letter and end with a period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's preferred. But what would be better here is more elaboration. Mostly, this comment nearly repeats what the code itself says, without adding much. I think this comment should:
- Say why this is being done.
- Include instruction on how to take advantage of it. If you're someone who is reading this
build.rs
trying to understand it, and we say, "this enables long file paths" without saying that it also requires a registry edit, that might produce some confusion. So let's shine a light where we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got through most of this PR review before realizing that it's using a dependency that I will not bring in. I'm sorry. Someone tried this before in #1898. Specifically, see my comment on that PR.
@@ -1,12 +1,12 @@ | |||
use clap::Shell; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this moved? Please put it back. :)
use app::{RGArg, RGArgKind}; | ||
extern crate embed_resource; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this should be removed.
@@ -48,6 +48,9 @@ fn main() { | |||
if let Some(rev) = git_revision_hash() { | |||
println!("cargo:rustc-env=RIPGREP_BUILD_GIT_HASH={}", rev); | |||
} | |||
|
|||
// embed windows application manifest file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's preferred. But what would be better here is more elaboration. Mostly, this comment nearly repeats what the code itself says, without adding much. I think this comment should:
- Say why this is being done.
- Include instruction on how to take advantage of it. If you're someone who is reading this
build.rs
trying to understand it, and we say, "this enables long file paths" without saying that it also requires a registry edit, that might produce some confusion. So let's shine a light where we can.
@@ -0,0 +1,2 @@ | |||
#define RT_MANIFEST 24 | |||
1 RT_MANIFEST "rg.exe.manifest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment on it, but where did the rg.exe.manifest
file come from? How was it generated?
Also, I think it would be better to put these files into a windows
sub-directory, instead of polluting the root of the repo even more than it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, GitHub falsely detects it as a binary file, but it's just plain text. Phew. That's good. Yeah, I think just move these into a new windows
sub-directory. Thanks!
@@ -48,6 +48,9 @@ fn main() { | |||
if let Some(rev) = git_revision_hash() { | |||
println!("cargo:rustc-env=RIPGREP_BUILD_GIT_HASH={}", rev); | |||
} | |||
|
|||
// embed windows application manifest file | |||
embed_resource::compile("rg-manifest.rc"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do on non-Windows targets? Even if nothing, it might be nice to gate this on #[cfg(windows)]
to make it a bit clearer to those reading the code.
@@ -61,6 +61,7 @@ version = "0.3.0" | |||
|
|||
[build-dependencies] | |||
lazy_static = "1.1.0" | |||
embed-resource = "1.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed for non-Windows builds right? Could you please make this a target specific dependency for windows only? Thanks!
@BurntSushi @SimplyDanny thank you for your feedback! Would an implementation based on https://crates.io/crates/windres (which is based on https://crates.io/crates/find-winsdk ) be acceptable for ripgrep? |
As long as there's nothing written by Jonathan Blow in the dependency tree, the licenses are compatible and the crates are decently maintained. Remember, once this gets added to ripgrep, it's forever mine to maintain, evev if the dependencies go defunct. So in short: probably, but I'll still need to do a review if a PR comes in. |
For anyone interested in long path support: Stumbled over this recent pull request for rustc which adds a similar manifest file to the rustc executable. It apparently works for the *-msvc target by using a build script without external libraries. |
Previously, the parsing was more limited and couldn't handle sub-paths in repos, like BurntSushi/ripgrep#2049. Now anything that includes an org/user & repo at the beginning of the path should work.
@ChrisDenton what are your thoughts on this PR? Is this the right way to support long paths on Windows? |
Yep. I'd be minded to append My only quibble is that it's a lot of dependencies for what can be a couple of lines in a build script (plus a few more lines to apply it conditionally). Though unfortunately windows-gnu does not support this. |
@ChrisDenton Oh yessss! Thank you for that link. I will do what you did for |
See the README and comments in the build.rs. Basically, this embeds an XML file that I guess is a way of setting configuration knobs on Windows. One of those knobs is enabling long path support. You still need to enable it in your registry (lol), but this will handle the other half of it. Fixes #364, Closes #2049
See the README and comments in the build.rs. Basically, this embeds an XML file that I guess is a way of setting configuration knobs on Windows. One of those knobs is enabling long path support. You still need to enable it in your registry (lol), but this will handle the other half of it. Fixes #364, Closes #2049
See the README and comments in the build.rs. Basically, this embeds an XML file that I guess is a way of setting configuration knobs on Windows. One of those knobs is enabling long path support. You still need to enable it in your registry (lol), but this will handle the other half of it. Fixes #364, Closes #2049
fixes #364
Implementation is based on https://gal.hagever.com/posts/windows-long-paths-in-rust/ .
This still requires long file paths to be enabled in the windows registry.