fix(win): Embed manifest in OIIO .exes to enable long path handling#5066
Conversation
| // The value is invariant, so should be safe to hardcode, but we could replace | ||
| // 24 with `RT_MANIFEST` by adding `#include "winres.h"` first. |
There was a problem hiding this comment.
This is something I don't have a strong instinct for. The manifest resource ID isn't going to change, and this value hardcoding seems to be fairly commonplace from what I've found. I opted to avoid an #include here to start just because I don't know if that would have any other implications.
|
Need a DCO sign-off on the commit. Can you add something to the testsuite that verifies that this allows the long filenames so we will know if there is any regression? |
54b05d2 to
1e67550
Compare
|
DCO should be fixed. I'll look at adding a test for this as soon as I can. |
3452a39 to
823fa21
Compare
|
OK, I've added a smoke test for long path support in However, the last thing I think I need to do is make the test conditional on whether long path support is enabled in the registry (the first of the two conditions documented here: https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry#enable-long-paths-in-windows-10-version-1607-and-later) I'll try to figure that out tomorrow (worst case scenario is I can just have the test script check and "succeed" if it is not enabled). |
What? You mean that in addition to the fix on our side, there's also a system setting not under our control that can be wrong, and will make our software just fail for long paths? |
|
By the way, if you rebase on current main, I think it will clear up the spurious CI failures. |
Yup. The additional key in the executable manifests only has an effect if long path support is enabled at the system level. Setting aside the insanity of this situation, I can imagine two schools of thought here from a maintainer's perspective:
|
Signed-off-by: Nathan Rusch <nrusch@users.noreply.github.com>
Signed-off-by: Nathan Rusch <nrusch@users.noreply.github.com>
823fa21 to
6e3caf7
Compare
|
I've added a check for the registry setting before enabling the test, and rebased onto |
I think we're doing exactly the right thing: Fix the part that's under our control as simply as we can, and for the rest, if somebody is using long filenames and Windows and don't know about the registry thingie, they've made their own bed. |
lgritz
left a comment
There was a problem hiding this comment.
LGTM, thanks for this fix!
Description
Adds an embedded manifest to OIIO's generated executables on Windows, which enables "long path" awareness.
This is done via a "resource" file, with the manifest resource ID specified. From what I've read, this is more broadly compatible with e.g. non-MSVC build toolchains like MinGW.
The manifest file was created by building OIIO from
main, extracting the manifest from the resultingoiiotool(using themttool), and manually adding in the XML snippet from https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry#application-manifest-updates-to-declare-long-path-capability.Fixes #5064
Tests
Added a new Windows-only test to validate that
oiiotoolcan correctly read long image paths.The one executable that I have not tested is
iv. The only reason I'm more curious about it than others is that it is a GUI executable, so I'm not sure if its defaultChecklist:
behavior.
testsuite.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.