-
Notifications
You must be signed in to change notification settings - Fork 649
build: Include Windows version information on produced binaries #4696
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
build: Include Windows version information on produced binaries #4696
Conversation
Signed-off-by: Jesse Yurkovich <[email protected]>
|
Thanks, I never would have known how to do this, or even that it was a thing. It looks fine to me, inasmuch as it's all enclosed in But I'm not qualified to know if you've done it right. Would anybody else like to get their eyes on this and weigh in? (I will merge it in a couple days unless somebody objects.) Should we have something in the testsuite (obviously that only runs for Windows) that validates this in some way? You posted a screenshot from a GUI tool, but is there anything scriptable that can extract that information from the libraries and compare to a reference output, so we can be sure via CI that we don't break this in the future? |
ThiagoIze
left a comment
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 looks essentially good to me. There's a few parts where I don't know enough about this to say if it's totally right. For instance, VarFileInfo is listed as 1252. Why that instead of 1200, which is unicode? I don't know the answer. Maybe it doesn't really matter for our purposes?
I'm approving on the assumption that this is working properly for you in Windows. I do like what the new properties window is showing.
It's trivially accessible using Powershell. Let me see if I can make it work somehow. |
It's a good question that I can't find a definitive answer to. The VarFileInfo block relates to the StringFileInfo block above it; i.e. the |
Signed-off-by: Jesse Yurkovich <[email protected]>
|
Alright, added a test to verify the embedded version information and that seems to pass. Here's 2 examples of what you might see from the output of the script during both a Pass and a Fail: Pass Fail |
lgritz
left a comment
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.
LGTM!
…emySoftwareFoundation#4696) This change embeds version information into our Windows binaries. This is done by ways of a `version_win32.rc` resource file[1] populated at CMake configure time with our major.minor.patch version information. This not only allows at a glance to discover what version a particular OIIO binary is (see screenshot), but it also impacts certain MSI installer behaviors. If this is a MSI install which updates a prior installation, and the shared libs/dlls don't have version information, then the installer assumes the binaries are equal and just skips them. This can be problematic for applications that package OIIO etc. Old on left, New on right:  [1] https://learn.microsoft.com/en-us/windows/win32/menurc/versioninfo-resource --------- Signed-off-by: Jesse Yurkovich <[email protected]>
…emySoftwareFoundation#4696) This change embeds version information into our Windows binaries. This is done by ways of a `version_win32.rc` resource file[1] populated at CMake configure time with our major.minor.patch version information. This not only allows at a glance to discover what version a particular OIIO binary is (see screenshot), but it also impacts certain MSI installer behaviors. If this is a MSI install which updates a prior installation, and the shared libs/dlls don't have version information, then the installer assumes the binaries are equal and just skips them. This can be problematic for applications that package OIIO etc. Old on left, New on right:  [1] https://learn.microsoft.com/en-us/windows/win32/menurc/versioninfo-resource --------- Signed-off-by: Jesse Yurkovich <[email protected]> Signed-off-by: Scott Wilson <[email protected]>
…emySoftwareFoundation#4696) This change embeds version information into our Windows binaries. This is done by ways of a `version_win32.rc` resource file[1] populated at CMake configure time with our major.minor.patch version information. This not only allows at a glance to discover what version a particular OIIO binary is (see screenshot), but it also impacts certain MSI installer behaviors. If this is a MSI install which updates a prior installation, and the shared libs/dlls don't have version information, then the installer assumes the binaries are equal and just skips them. This can be problematic for applications that package OIIO etc. Old on left, New on right:  [1] https://learn.microsoft.com/en-us/windows/win32/menurc/versioninfo-resource --------- Signed-off-by: Jesse Yurkovich <[email protected]> Signed-off-by: Scott Wilson <[email protected]>
Description
This change embeds version information into our Windows binaries. This is done by ways of a
version_win32.rcresource file[1] populated at CMake configure time with our major.minor.patch version information.This not only allows at a glance to discover what version a particular OIIO binary is (see screenshot), but it also impacts certain MSI installer behaviors. If this is a MSI install which updates a prior installation, and the shared libs/dlls don't have version information, then the installer assumes the binaries are equal and just skips them. This can be problematic for applications that package OIIO etc.
Old on left, New on right:

[1] https://learn.microsoft.com/en-us/windows/win32/menurc/versioninfo-resource
Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.