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.

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

CMake: Fix issue that led to Windows Unicode support still defaulting to off #1594

Merged

Conversation

itsmattkc
Copy link
Contributor

Follow-up from #1363

In that PR, a CMake option was introduced called OCIO_USE_WINDOWS_UNICODE to allow users to compile either an ANSI-only or Unicode build on Windows. Its default value was set to WIN32 in an attempt to automatically enable on Windows and disable on other platforms.

It turns out there's a syntax issue here, CMake sets it to the literal string "WIN32" rather than the value of the variable ${WIN32}, and it considers that string literal equivalent to OFF at all times.

This could be corrected by simply changing the default value to ${WIN32} instead of WIN32, which I've tested and works correctly, however what I've written in this PR is a wrapping of that CMake option in an if (WIN32) and defaulting the option toON. Personally, I prefer this approach because I think the code is more readable and there's no real point for a Windows-only option being visible/available on non-Windows platforms anyway. However, if the former approach is preferred, I can write that in instead.

Signed-off-by: itsmattkc <34096995+itsmattkc@users.noreply.github.com>
Copy link
Collaborator

@remia remia left a comment

Choose a reason for hiding this comment

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

Thanks @itsmattkc, I can confirm this works correctly on my Windows box and think it's easier to read that way as well.

@doug-walker doug-walker merged commit 73508eb into AcademySoftwareFoundation:main Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants