Page MenuHome

[Cycles/MSVC/Testing] Fix broken test code.
ClosedPublic

Authored by LazyDodo (LazyDodo) on Nov 18 2016, 10:00 PM.

Details

Summary

Currently the tests don't run on windows for the following reasons

  1. render_graph_finalize has an linking issue due missing a bunch of libraries (not sure why this is not an issue for linux)
  2. This one is more interesting, in test/python/cmakelists.txt ${TEST_BLENDER_EXE_BARE} and ${TEST_BLENDER_EXE} are flat out wrong, but for some reason this doesn't matter for most tests, cause ctest will actually go out and look for the executable and fix the path for you *BUT* only for the command, if you use them in any of the parameters it'll happily pass on the wrong path.
  3. on linux you can just run a .py file, windows is not as awesome and needs to be told to run it with pyton.
  4. had to use the NAME/COMMAND long form of add_test otherwise $<TARGET_FILE:blender> doesn't get expanded, why? beats me.
  5. missing idiff.exe for msvc2015/x64 in the libs folder.

This patch addresses 1-4 , but given I have no working Linux build environment, I'm unsure if it'll break anything there

5 has been fixed in rBL61751

Diff Detail

Repository
rB Blender

Event Timeline

LazyDodo (LazyDodo) retitled this revision from to [Cycles/MSVC/Testing] Fix broken test code..Nov 18 2016, 10:00 PM
LazyDodo (LazyDodo) updated this object.
LazyDodo (LazyDodo) set the repository for this revision to rB Blender.
LazyDodo (LazyDodo) added projects: Cycles, Restricted Project.
LazyDodo (LazyDodo) updated this revision to Diff 7849.
intern/cycles/test/CMakeLists.txt
39

Is that really needed?

AFAIK, we wanted to make it so all FOO_LIBRARY are absolute paths.

tests/python/CMakeLists.txt
404

Not sure why this is only needed for Cycles tests. There are other tests which are using TEST_BLENDER_EXE. So mu guess is that they will also fail and that we should have something done at the beginning of the file where we calculate TEST_BLENDER_EXE.

intern/cycles/test/CMakeLists.txt
39

Copied this from the creator cmakelists.txt, atleast for windows most paths are absolute, but some of them are just the libname. (libpng/openimageio/boost) png and openimageio i can probably convert to full paths, boost given it's depending on the system findboost.cmake seems unlikely. so we'll still need atleast that.

tests/python/CMakeLists.txt
404

See point 2, the cycles tests are the only ones that use the TEST_BLENDER_EXE_BARE as a parameter, if it's the command, ctest sighs and fixes the path silently for you, but not for parameters.

intern/cycles/test/CMakeLists.txt
39

Interesting.

Well, guess we'll worry about converting libs to abspath later.

tests/python/CMakeLists.txt
404

Wouldn't it make sense to set TEST_BLENDER_EXE to "$<TARGET_FILE:blender>" so we don't care how we use the command?

tests/python/CMakeLists.txt
404

I tried, but cmake refuses to expand it, I had to use $<TARGET_FILE:blender> if i used

set(TEST_BLENDER_EXE $<TARGET_FILE:blender>)
set(TEST_BLENDER_EXE_BARE ${TEST_BLENDER_EXE})

 add_test(cycles_render_test
            COMMAND python ${CMAKE_CURRENT_LIST_DIR}/cycles_render_tests.py
            -blender "${TEST_BLENDER_EXE_BARE}"
            -testdir "${TEST_SRC_DIR}/cycles/ctests/render"
            -idiff "${OPENIMAGEIO_IDIFF}"
)

it resulted in the following command it tried to execute

20: Test command: python.exe "K:/BlenderGit/blender/tests/python/cycles_render_tests.py" "-blender" "$<TARGET_FILE:blender>" "-testdir" "K:/BlenderGit/blender/../lib/tests/cycles/ctests/re
nder" "-idiff" "K:/BlenderGit/blender/../lib/win64_vc14/openimageio/bin/idiff.exe"

which naturally failed badly.

Sergey Sharybin (sergey) accepted this revision.
Sergey Sharybin (sergey) added inline comments.
tests/python/CMakeLists.txt
404

Weirdly it worked in cmake-3.6.2 but not in cmake-3.7.1 for me.

Well, guess better to solve the issue for now and dig into cmake specifics later.

This revision is now accepted and ready to land.Dec 8 2016, 3:50 PM
LazyDodo (LazyDodo) removed rB Blender as the repository for this revision.
LazyDodo (LazyDodo) updated this revision to Diff 7976.

While running sanity checks before committing this diff, more problems showed up

  1. When building with msvc2015/BLI_string_test , EXPECT_EQ_ARRAY_ND can't pick the right template between int and const int , causing a build error.
  2. Soo many linker errors when using msvc2015,with intern/cycles/test/CMakeLists.txt , at a closer look intern/cycles/test/CMakeLists.txt is redoing pretty much all the work setup_liblinks was already doing (but poorly, was linking in some release libs into debug builds causing chaos), no need to have the same code in 2 places. removed it.
  3. OpenSubDiv weirdness in setup_liblinks , it tries to be clever and figure out the debug libs, not sure why we handled this this way, all other libs are managed in platform_win32_msvc.cmake fixed the problem there and removed the exception in setup_liblinks.
  4. Some tests require additional shared libraries (bmesh_core_test for example, but there's others) those failed, because they simply couldn't start, changed the working directory for tests to be the blender output folder, where those dll's wil be guaranteed to be found.
  5. Windows doesn't have python build in or as it is on my local system python2 is found in the path, our script have some python 3 code in them. counting on python3 in the path is a bit annoying, we know where we can find a python3 for sure(2.78/python/bin) since we ship it ourselves.
  6. CTestTesting.cmake was lacking ${GFLAGS_DEFINES} ${GLOG_DEFINES} in causing linker errors with msvc2015

I validated all these changes against msvc 2013/2015 x64 all tests run now(good), but 4 fail (bad, but this diff is about getting the tests working, not passing)

I know this diff has been accepted already , but given i've done some rather big changes I'd like a sober second look (and perhaps a linux user to take a quick peek and check if haven't broken anything)

Sergey Sharybin (sergey) requested changes to this revision.
Sergey Sharybin (sergey) added inline comments.
build_files/cmake/Modules/GTestTesting.cmake
29

This doesn't seems a good place to have this. BLENDER_SRC_GTEST_EX is a macro, meaning all modifications ot the environment are propagated to the parent scope. This means, that if you have multiple subsequent calls to this macro you'll be adding GFlags/GLog defines again and again.

My guess is that you should:

  • Revert this chunk of change
  • Remove add_definitions() from tests/gtests/testing/CMakeLists.txt
  • Add this defines to tests/gtests/CMakeLists.txt
intern/cycles/test/CMakeLists.txt
11

You can't do this from within Cycles directory since it should be kept snadalone-alike. There is no such macro in Cycles repository and and wouldn't want to being WHOLE blender-specific libraries linking thingie to a standalone Cycles repo.

15

Same as above.

tests/gtests/blenlib/BLI_string_test.cc
371

I don't luck such a compiler idiocy and don't like discarding const qualifier just to make something compiled.

Would that work if we make it const T *actual? It's not like we are modifying it in the EXPECT function anyway.

This revision now requires changes to proceed.Dec 12 2016, 9:52 AM
LazyDodo (LazyDodo) updated this revision to Diff 7996.

I'm open for an alternative for the monstrosity that is the python path, it's the only way i found that supported both the release and debug version on windows, while also not breaking on linux.

Sergey Sharybin (sergey) requested changes to this revision.
Sergey Sharybin (sergey) added inline comments.
tests/python/CMakeLists.txt
404

The new code is wrong here for linux. It's no t guaranteed to have /use/bin/python to point to a python3 install. Can we leave it up to shebang to decide which python to use? Alternatively, we should make it /usr/bin/python3.

This revision now requires changes to proceed.Jan 25 2017, 2:44 PM
tests/python/CMakeLists.txt
404

for linux it's just calling 'python' with no path like the original script did. the only bit that applies for linux is

$<$<STREQUAL:${MSVC},>:python>

(if MSVC not defined->python)

the rest of this abomination falls in the MSVC defined clause

tests/python/CMakeLists.txt
404

Original CMake did not have any explicit mention of python and simply called ${CMAKE_CURRENT_LIST_DIR}/cycles_render_tests.py, leaving actual intepreter to be deducted from shebang. Now the CMake calls python which gets expanded to /usr/bin/python which is not necessarily a python3.

tests/python/CMakeLists.txt
404

Ah yikes, the only way to restore the old functionality would be something like

COMMAND "$<$<STREQUAL:${MSVC},1>:$<TARGET_FILE_DIR:blender>/${BLENDER_VERSION_MAJOR}.${BLENDER_VERSION_MINOR}/python/bin/python$<$<CONFIG:Debug>:_d>>$<$<STREQUAL:${MSVC},>:${CMAKE_CURRENT_LIST_DIR}/cycles_render_tests.py>" $<$<STREQUAL:${MSVC},1>:${CMAKE_CURRENT_LIST_DIR}/cycles_render_tests.py>

but this is getting ridiculous , i think i'm gonna have another stab at using the blender.exe as the python interpreter

LazyDodo (LazyDodo) set the repository for this revision to rB Blender.
LazyDodo (LazyDodo) updated this revision to Diff 8180.

Updated with your macro, much nicer.

This revision is now accepted and ready to land.Jan 25 2017, 5:29 PM
This revision was automatically updated to reflect the committed changes.