Skip to content

Conversation

@ericmehl
Copy link
Collaborator

@ericmehl ericmehl commented Jul 8, 2022

This restores Windows builds. When Github removed support for MSVC 2017, a new build was needed with MSVC 2019, which included numerous dependency updates.

With the Windows dependencies at parity with the HQ dependencies, these updates get Cortex building and testing again on Windows.

I'm targeting main for now, though depending on the release outlook, it should I target 10.4 ( which is identical to main at this moment)?

Breaking Changes

None

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Cortex project's prevailing coding style and conventions.
  • If my code made breaking changes, I applied the pr-majorVersion label to this PR.

ericmehl added 10 commits July 6, 2022 18:45
- Dependencies now use simplified Boost packaging in https://github.com/hypothetical-inc/gafferDependencies/releases/tag/5.0.0, making the file and directory names less verbose.
- Move `CXXFLAGS` into `SConstruct`. It's not possible to set a list value from a batch script, which is what the dependencies project uses to build Cortex. Also these flags are required, so this move makes them less prone to being left out.
- USD 21.11 prefixes libraries with `usd_`
- Warnings were being generated by Boost::Wave :
        include\boost\wave\util\cpp_macromap.hpp(1819) : warning C4701: potentially uninitialized local variable 'is_system' used
        include\boost\wave\util\cpp_macromap.hpp(1819) : warning C4701: potentially uninitialized local variable 'is_quoted_filename' used
        include\boost\wave\util\cpp_macromap.hpp(1819) : warning C4701: potentially uninitialized local variable 'is_system' used
        include\boost\wave\util\cpp_macromap.hpp(1819) : warning C4701: potentially uninitialized local variable 'is_quoted_filename' used
- Rename second `b` variable and expand bound calculation in loop.
- MSVC seems to be inconsistent with considering headers included via double quotes as external. Rather than breaking with convention and including external dependencies in angle brackets, we disable these warnings until MSVC handles external warnings correctly.
- See https://developercommunity.visualstudio.com/t/analyze:external--and-external:I-flags/1688240#T-N1689542 for additional information. They are using `/analyze` but it seems to apply to compiling as well.
…for function-like macro invocation

- This came back after upgrading to MSVC 2019 and the latest USD. Despite USD trying to handle suppressing this warning, it seems to be stubborn and continues to be raised without further efforts.
- Using `ARCH_PRAGMA_*` is a bit cleaner and gracefully degrades to a no-op on non-Windows builds.
…nversion )

- The constructor for `half` only takes a `float` value, causing an implicit cast and warning 4244 when casting a `double` to `half`. Since this function is only being called for `unsigned` types, we can safely replace `std::numeric_limits::min` with 0.
- Update Window dependencies to 6.0.0.
…ndows

- Recent versions of `OpenImageIO` corrupt data during color conversion with built-in conversions. Using `OpenColorIO` fixes color conversion.
@ericmehl ericmehl force-pushed the windows-dependencies-6.0.0 branch from 0abfbcd to c55d4e2 Compare July 8, 2022 20:41
Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Thanks Eric, great to have Windows back! I think the couple of minor comments I made are probably worth dealing with, but otherwise LGTM. And yep, we should target this to RB-10.4 now as well - that's the branch that will continue to feed into Gaffer 1.0.x.

ericmehl added 2 commits July 11, 2022 21:17
USD is inconsistent on the case of the drive letter when returning Windows paths. Windows is not case-sensitive, so we can safely correct case in tests.
Removing unreferenced data or functions that only have internal linkage causes errors when USD tries to load a plugin such as the SCC loader in IECoreUSD. See PixarAnimationStudios/OpenUSD#1095 for more discussion.
@ericmehl ericmehl force-pushed the windows-dependencies-6.0.0 branch from c55d4e2 to cb1cfd5 Compare July 12, 2022 01:18
@ericmehl
Copy link
Collaborator Author

Except for a timing glitch on the Linux side, these are passing with the fixups to the tests. Ready for a new look.

@johnhaddon johnhaddon changed the base branch from main to RB-10.4 July 12, 2022 15:18
@johnhaddon johnhaddon merged commit 2dcd943 into ImageEngine:RB-10.4 Jul 12, 2022
@johnhaddon
Copy link
Member

Thanks Eric - I've switched this to target RB-10.4 and merged.

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.

2 participants