Page MenuHome

CTest: Fix Blender_Test on windows
ClosedPublic

Authored by Ray Molenkamp (LazyDodo) on Jul 27 2020, 9:11 PM.

Details

Summary

I am aware nobody is going to like this, so if anyone has a better idea, I'm open to suggestions.

Ever since D7649 landed there have been issues on windows, Initially flags for whole archive linking were not set (fixed in D8404) leading to no tests getting linked, but now that the executable properly builds and links we run into the following scenario:

  1. gtest_discover_tests runs as a post build command (for blender_test) to get a list of tests embedded in the blender_test binary
  2. This test depends on half of blender, and needs virtually all dlls we have to be inplace before it will run
  3. These dlls will not get copied until the install phase of the blender target

Which is problematic, now we are seemingly not the first to run into this and in cmake 3.18 they added a DISCOVERY_MODE option for gtest_discover_tests which allows you to move the discovery of the tests to the PRE_Test phase.

however this would require a cmake version bump to 3.18 (Jul 15 2020) for windows.

Somewhat related, we have been playing it very fast and lose with our cmake support, the use of gtest_discover_tests raises the minimal cmake version to at-least 3.10 for all other platforms, yet our cmake_minimum_required is still set to 3.5 which is a straight up lie at this point, we really should re-evaluate this.

Diff Detail

Repository
rB Blender
Branch
tmp_multiconfig (branched from master)
Build Status
Buildable 9220
Build 9220: arc lint + arc unit

Event Timeline

Ray Molenkamp (LazyDodo) requested review of this revision.Jul 27 2020, 9:11 PM

Think it is fine to bump CMake requirement on Windows. Is super simple to install new one.
Linux is another story, but seems that we don't need to require a newer CMake there yet.

tests/gtests/runner/CMakeLists.txt
115

Does this mean that actually all platforms need CMake 3.18? Or does it still work with older CMake's?

Linux is another story, but seems that we don't need to require a newer CMake there yet.

Well, because of D7649 we still need to bump to 3.10.

tests/gtests/runner/CMakeLists.txt
115

It also works on CMake 3.16 (I tested with that). From what I can see unknown arguments are just ignored; I could add other nonsense arguments there without any error.

Well, because of D7649 we still need to bump to 3.10.

Fine by me. CentOS is 3.17.3, which you can install very easily. On mac you always pretty much on the bleeding edge cmake, on windows is simple to install the latest.

So i'm fine with bump to 3.10 for mac/linux and 3.18 on windows.

tests/gtests/runner/CMakeLists.txt
115

Nice :)

This revision is now accepted and ready to land.Jul 28 2020, 1:30 PM
  • Fix test runner for multi configuration generators.
  • Fix Single config generator on windows.
Ray Molenkamp (LazyDodo) requested review of this revision.Jul 28 2020, 9:15 PM

Allright, this was A LOT messier than it seemed at first sight, problem is you can't have any cmake variables in the CMAKE_INSTALL_PREFIX variable due to it running at test time, this problem is not exclusive to windows, any multi configuration target should be affected by this.

I have tested on windows with MSBuild and Ninja in our regular configurations (make.bat) and with the buildbot config (which is somewhat of an odd duck with its install prefix pointing out of the build folder)

I cannot test mac (which should have the same issue if cmake 3.18 is present) and I have not tested linux.

what i'm saying is, this needs a re-review and be tested on mac/linux

tests/gtests/runner/CMakeLists.txt
79–111

Can you use TEST_INSTALL_DIR that is set in tests/CMakeLists.txt?

I'd expect that finding the Blender executable location is the same problem as the finding the test runner.

tests/gtests/runner/CMakeLists.txt
79–111

Urgh, I actually came really close to what TEST_INSTALL_DIR is doing but messed up the casing and used $<Config> which did not work, TEST_INSTALL_DIR seems to do the trick but i'll have to do some validation builds to check all scenarios are covered.

Nice find! thanks!

  • Use TEST_INSTALL_DIR
This revision is now accepted and ready to land.Jul 30 2020, 8:34 PM
  • restore original CMAKE_INSTALL_PREFIX code