[Draft] Windows: Add longPathAware manifest file
#1898
Conversation
Adds a manifest file which can be used to enable support for long paths, without needing any special handling in the application's code.
|
cc @ehuss Do you have any thoughts on how to do this better? Or is |
There was a problem hiding this comment.
Amaaazing! I've been hoping someone with more Windows knowledge than myself would put something like this together. If cargo rustc is indeed the best we can do, then I definitely think this is still worth doing and adding a note in the README for it. Ideally, we could provide some nicer way of building without requiring the user to pass linker arguments.
I would also request that we create a new top-level directory called windows and drop this file there, instead of putting it at the top-level itself. Putting a README in that directory explaining the files would be bonus points. :-)
This still requires the ripgrep user to also enable long path support.
I think we will want to include these instructions in the README. The fact that a user has to edit their registry for this seems absolutely bonkers to me though.
Either way, it'd also be good to integrate it with the ci but I'm unsure how best to do that.
I think we want it in both the normal build workflow and the release workflow. For the release workflow for example, I think you'll want to add a if: matrix.os != 'windows-2019' to this block, and then add another block with if: matrix.os == 'windows-2019' that uses cargo rustc instead. Modifying the ci workflow will be more tedious, but I think it's the same idea. Maybe splitting the build command to an env var (cargo build by default, cargo rustc for Windows MSVC) would be better. Similarly for the release workflow. (And maybe my suggestion above is wrong, because it would try to use this manifest with the GNU builds too.)
WinManifest.xml
Outdated
| <requestedExecutionLevel level="asInvoker" uiAccess="false" /> | ||
| </requestedPrivileges> | ||
| </security> | ||
| </asmv3:trustInfo> |
There was a problem hiding this comment.
Is this stanza necessary? What happens if we don't include it?
There was a problem hiding this comment.
Sure, we can exclude it. The linker will add it back anyway.
There was a problem hiding this comment.
Great. General philosophy here is to just keep this file as small as possible so that it does the absolute minimum thing required.
WinManifest.xml
Outdated
| <application> | ||
| <!-- Windows 7 --><supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/> | ||
| <!-- Windows 8 --><supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/> | ||
| <!-- Windows 10 --><supportedOS Id="{8e0f7a12-bfb3-4fe8-b9a5-48fd50a15a9a}"/> |
There was a problem hiding this comment.
Could you add a comment explaining how to update this stanza when a new version of Windows comes out?
WinManifest.xml
Outdated
| <!-- Allows using longer paths without special handling --> | ||
| <!-- See: https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=cmd#enable-long-paths-in-windows-10-version-1607-and-later --> | ||
| <ws16:longPathAware>true</ws16:longPathAware> | ||
| <ws19:activeCodePage>UTF-8</ws19:activeCodePage> |
There was a problem hiding this comment.
Will this have any harmful effects for versions of Windows older than Windows 10 v 1607?
There was a problem hiding this comment.
In that case it will be unrecognised so it should be ignored. I will test on a Windows 7 VM to double check.
WinManifest.xml
Outdated
| <assemblyIdentity | ||
| type="win32" | ||
| name="BurntSushi.github.ripgrep" | ||
| version="13.0.0.0" |
There was a problem hiding this comment.
Is this required? I try not to put the version number in too many places. Right now, it's in exactly two places, and in the most recent release, I forgot to update one of them.
If it's unavoidable, then I think adding a step to RELEASE-CHECKLIST.md would be the right thing to do.
There was a problem hiding this comment.
Hmm, I'm actually unsure here. The docs list it as "required" but excluding assemblyIdentity entirely still seems to work. I was just a bit nervous about breaking with the docs.
EDIT: In any case, I don't think the version actually has to be accurate or meaningful. It could just be set to 0.0.0.0
There was a problem hiding this comment.
Let's go with 0.0.0.0 for now until someone complains with a good reason to change what we're doing. :-)
|
Couldn't you use something like https://crates.io/crates/embed-resource to avoid the explicit flag requirement? |
|
Yeah, that might be easier! It would save having to mess about with the linker. I'll try it. |
|
@lespea I personally don't know anything about this, so "couldn't you use" is kind of framing this in the wrong direction. I don't know what we can and can't use. I'm not an expert here. With that said, looking at |
|
@BurntSushi oh sorry that was mostly directed at Chris. I haven't used embed-resource personally I just remember coming across it and kept it in the back of my mind for the future. Afaik it should only be used during build and wouldn't add anything to the binary itself, but I'm no expert in the matter... I was just trying to make the builds smoother for windows. |
|
Yeah, I'd rather not use it. The whole thing is built on something written by someone I do not trust. I'm not going to say anything more on that matter publicly. (And the README specifically warns about memory corruptions.) |
|
Ok, second draft. I've added the command to I've started writing some documentation but it could probably use some more work. I've also tentatively added some small utility files to make things a bit easier for users.
|
I'm not too familiar with embedding Windows manifests. Long-file path can be tricky (for example, AFAIK Rust's You can try using RUSTFLAGS (either the environment variable or config file), but that can have issues. There is an unstable feature for setting flags like these. It is described here, and essentially you have a build.rs script that prints |
Imho, that's an issue with Rust's current implementation. It can be fixed, although doing so isn't as trivial as I'd like,
That would be ideal! It would greatly simplify things when stabilized. @BurntSushi would it be worth pausing this PR until that feature is ready? |
|
@ChrisDenton Yeah, let's post-pone this until we have a better path here. This has been a long-standing bug, so I think we can wait a bit longer. With respect to the So I guess I'll close this for now. I am now also subscribed to rust-lang/cargo#9426, which appears to be the issue tracking the stabilization of passing link arguments through |
This adds a manifest file which enables support for long paths, without needing any special handling in the application's code.
Building with this manifest requires the following black magic (and an MSVC compatible linker):
The PR is a draft because:
Limitations
This still requires the ripgrep user to also enable long path support. See: Enable Long Paths in Windows 10, Version 1607, and Later.
As far as I know, the commands must be typed on the command line because
.cargo/config.tomland therustflagsenvironment variable pass link args to all linker invocation. There's no way to apply them only for a particular binary.