AL unit tests fixes for all three platforms

[HamedSabri-adsk]

Hi guys,

This pull request addresses various issues with running AL unit tests on different platform and both debug and release variants. In doing so, we hit various challenges on Windows that we had to tackle (e.g mix of "\" and "/" in temporary directories, whitespaces, linking against static GTest library, etc...).

With the current fixes, one should be able to run all AL tests from command line (Windows command prompt or bash).

We still have two tests that are failing on Linux and Windows (TestAdditionalTranslators and TestPxrUsdTranslators) which we would like to fix.

On MacOSX, all the tests are now passing.

➜ ctest -j 8                         
  Start 4: AL_USDMayaTestPlugin
  Start 7: TestAdditionalTranslators
  Start 5: TestUSDMayaPython
  Start 8: TestPxrUsdTranslators
  Start 1: AL_MayaUtilsTests
  Start 3: Python:AL_USDTransactionTests
  Start 2: GTest:AL_USDTransactionTests
  Start 6: testMayaSchemas
1/8 Test #2: GTest:AL_USDTransactionTests .....  Passed  0.06 sec
2/8 Test #6: testMayaSchemas ..................  Passed  0.09 sec
3/8 Test #3: Python:AL_USDTransactionTests ....  Passed  0.71 sec
4/8 Test #1: AL_MayaUtilsTests ................  Passed  5.90 sec
5/8 Test #8: TestPxrUsdTranslators ............  Passed  9.54 sec
6/8 Test #5: TestUSDMayaPython ................  Passed  10.32 sec
7/8 Test #7: TestAdditionalTranslators ........  Passed  12.24 sec
8/8 Test #4: AL_USDMayaTestPlugin .............  Passed  26.52 sec

100% tests passed, 0 tests failed out of 8

I am going to add comments on the line of code on github sometimes tomorrow to hopefully make it easier to review this PR. We also tried to keep the changes to minimal but still ended up with 57 files changes.

Change log:

- Port AL_MayaUtilsTests to Windows + runtime path fixes for MacOSX/Linux.
- Port AL_USDMayaTestPlugin to Windows + runtime path fixes for MacOSX/Linux.
- Port testMayaSchemas to Windows + runtime path fixes for MacOSX/Linux.
- Fix UnitTestHarness return status
- Fix runtime path for _AL_USDMaya on MacOSX
- Fix a Python bug in TestAdditionalTranslators
- Fix Python module extension on MacOSx + Python:AL_USDTransactionTests
- Build GTest as shared library on all platforms.
- Fix various issues with creating temporary files on Windows (WIP)
- Fix "Argument not separated from preceding token by whitespace" warning on Windows
- Fix a bug in fetch_googletest macro
- Use RUNPATH instead of RPATH for all shared libs and executables on Linux
- Clean up various Cmake files

[HamedSabri-adsk]

wrap all the full path variables around quotes in case there are located in weird places with whitespaces and special characters on Windows. Just to be safe!

[seando-adsk]

I have a couple of minor comments.

[seando-adsk]

When I was making my python debug changes I didn't realize this was a python library because it didn't have the .pyd extension. Good catch. But can you also make the change to add the _d to the name. Simply replace line 9 above with this:

if(IS_WINDOWS AND ${MAYAUSD_DEFINE_BOOST_DEBUG_PYTHON_FLAG})
# On Windows when compiling with debug python the library must be named with _d.
set(USDTRANSACTION_PYTHON_LIBRARY_NAME _${USDTRANSACTION_LIBRARY_NAME}_d)
else()
set(USDTRANSACTION_PYTHON_LIBRARY_NAME _${USDTRANSACTION_LIBRARY_NAME})
endif()

[HamedSabri-adsk]

Roger that!

[HamedSabri-adsk]

All the bash scripts for setting and running python unit tests are now removed for the sake of portability.

CMake PROPERTY ENVIRONMENT can be used instead to set ENV variables during the unit tests run.

[HamedSabri-adsk]

wrap all the full path variables around quotes in case there are located in weird places with whitespaces and special characters on Windows. Just to be safe!

[kxl-adsk]

@elrond79 I believe you are referring to missing __cxa_throw_bad_array_new_length symbol. We looked into it to understand what changed in this PR. @HamedSabri-adsk discovered that it's caused by the switch from python to mayapy in test commands.

As per previous comments, this missing symbol is not a new problem. We hit it already when fixing Pixar tests. mayapy is our way to have consistency across platforms for running tests.

We need tests working in the dev branch across all platforms and this last issue doesn't convince me we should keep delaying the merging. @murphyeoin - I will wait for your input on the subject since you reported it.

[pmolodo]

No, I was referring to this:

#198 (review)

Also, when running tests, the Python:AL_USDTransactionTests failed for me, because the _AL_USDTransaction.so python lib couldn't find libAL_USDTransaction.so. Took a look at the rpath, and saw that the first element was set incorrectly. Don't have time to dig further now.

[murphyeoin]

Happy to approve this, and assuming we'll soon find a fix to the python/DTS error discussed below

[pmolodo]

Decided to just approve for now, despite the rpath issue. Will make a separate issue for that after.
In the future, though, we need to avoid having such unnecessarily large PRs, to prevent these all-or-nothing scenarios.

[kxl-adsk]

@elrond79 FYI. We tried on multiple Linux setups and couldn't reproduce your rpath issue. Animal Logic didn't hit this issue either in Linux docker (which runs build.py).

[pmolodo]

@kxl-adsk Thanks for checking! I looked at it again, and figured out what's causing it - it has to do with how it handles symlinks in the build path, so that explains why others didn't see it.

I'll make a PR for it shortly.

[murphyeoin]

I know this PR is closed, but I wanted to follow up on the linker DTS issue on Linux again - this is proving to be a real pain for us in our standardised docker build.. to a point where I'm considering re-introducing as an internal patch the shell script that calls maya batch and runs the python script via a mel call... Wondering if anyone has tried building their own simple mayapy replacement that just evaluates the python script/module being passed in (without necessarily mimicking the full functionality of the python executable, just for executing tests? Would it suffer from the same problems?

[kxl-adsk]

We explored multiple potential solutions to the problem and nothing came as close to complete solution on all platforms as what we currently have with mayapy.

We don't recommend making the patch your described. Consider the following potential solutions instead:

1. Apply the patch for the missing symbol in USD
2. Run the failing tests through Maya shell (aka batch)
3. See comment from Paul

How to 1)
Create pxr/base/lib/vt/devtoolset6Workaround.cpp file with following content

#if (__GNUC__ >= 6)
#include <cstdlib>#pragma weak __cxa_throw_bad_array_new_lengthextern "C" void
__cxa_throw_bad_array_new_length()
{
 abort();
}
#endif

Then apply this patch:

diff --git a/pxr/base/lib/vt/CMakeLists.txt b/pxr/base/lib/vt/CMakeLists.txt
index aecffd7fb..648cc3013 100644
--- a/pxr/base/lib/vt/CMakeLists.txt
+++ b/pxr/base/lib/vt/CMakeLists.txt
@@ -39,6 +39,9 @@ pxr_library(vt
 PRIVATE_HEADERS
 typeHeaders.h+ CPPFILES
+ devtoolset6Workaround.cpp
+
 PYTHON_CPPFILES
 moduleDeps.cpp

How to 2)
Gtest test

    add_test(
        NAME ${TARGET_NAME}
        WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
        COMMAND "${MAYA_EXECUTABLE}" -batch -command "loadPlugin AL_USDMayaTestPlugin; \
                                                      int $res = `AL_maya_test_UnitTestHarness`; \
                                                      quit -f -exitCode `abs($res)`; \
                                                     "
    )

Python test

    add_test(
        NAME ${TEST_NAME}
        WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
        COMMAND "${MAYA_EXECUTABLE}" -batch -command "python(\"execfile(\\\"${CMAKE_CURRENT_SOURCE_DIR}/testRunner.py\\\")\")"
    )

[murphyeoin]

Hi @kxl-adsk - thanks for that... I had already seen Paul's comment..... 2) was mostly what I was proposing to do - but I'm glad there's a solution without the extra .sh script - which is great.... Will let you know how that goes and potentially put up a PR

[pmolodo]

If you want to make a global change to the way tests are handled, you might want to checkout PR#242: #242 That way, you'd only need to make your change in one place...

…

On Thu, Feb 6, 2020 at 3:35 PM Eoin Murphy ***@***.***> wrote: Hi @kxl-adsk <https://github.com/kxl-adsk> - thanks for that... I had already seen Paul's comment..... 2) was mostly what I was proposing to do - but I'm glad there's a solution without the extra .sh script - which is great.... Will let you know how that goes and potentially put up a PR — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#198?email_source=notifications&email_token=AAARQ6HZPTNEOMEXNP6PYRLRBSNDXA5CNFSM4KGLBTR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELBGADY#issuecomment-583163919>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAARQ6BBM5VEJXSPZT3YUGDRBSNDXANCNFSM4KGLBTRQ> .

[pmolodo]

Or - well, 1 place for all the AL + autodesk tests - as Krystian pointed out, the pxr tests still go through their own system.

…

On Thu, Feb 6, 2020 at 3:38 PM Paul Molodowitch ***@***.***> wrote: If you want to make a global change to the way tests are handled, you might want to checkout PR#242: #242 That way, you'd only need to make your change in one place... On Thu, Feb 6, 2020 at 3:35 PM Eoin Murphy ***@***.***> wrote: > Hi @kxl-adsk <https://github.com/kxl-adsk> - thanks for that... I had > already seen Paul's comment..... 2) was mostly what I was proposing to do - > but I'm glad there's a solution without the extra .sh script - which is > great.... Will let you know how that goes and potentially put up a PR > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#198?email_source=notifications&email_token=AAARQ6HZPTNEOMEXNP6PYRLRBSNDXA5CNFSM4KGLBTR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELBGADY#issuecomment-583163919>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAARQ6BBM5VEJXSPZT3YUGDRBSNDXANCNFSM4KGLBTRQ> > . >

[kxl-adsk]

We already explored 2) and it doesn’t apply to all platforms, so it’s not a direction we want to go with.

[murphyeoin]

@elrond79 thank you that looks fantastic...
@kxl-adsk - am slightly confused - why is option 2) integrated into #242 not applicable to all platforms? If this is the case, we could add a conditional for linux (or refine that if we can work out if we're using the DTS setup/only applies to certain compiler versions etc) vs other platforms?

[kxl-adsk]

We have to run tests with mayapy on MacOS. Having different methods of running tests per platform is not where we want to be.

[murphyeoin]

I agree that's not where we want to be - and in an ideal world we wouldn't be there - but if Linux tests can't be run by anyone using the recommended VFX Platform setup - surely that's a bigger problem?

[kxl-adsk]

Option 1) allows you to run all tests using recommended VFX Platform setup. Why you don’t want to try it?

[murphyeoin]

Updating a whole docker pipeline is time consuming for us - but it can be done. The bigger question is that everyone will have to do the same - whereas solution 2 will be transparent

[murphyeoin]

Anyway, we will give #1 a try, will take til Monday before all of the docker builds run and go through..here's the correct patch (I think the formatting in the one above went bad?):

diff --git a/pxr/base/lib/vt/CMakeLists.txt b/pxr/base/lib/vt/CMakeLists.txt
index aecffd7fb..c8f840bed 100644
--- a/pxr/base/lib/vt/CMakeLists.txt
+++ b/pxr/base/lib/vt/CMakeLists.txt
@@ -38,6 +38,9 @@ pxr_library(vt
 
     PRIVATE_HEADERS
         typeHeaders.h
+        
+    CPPFILES
+        devtoolset6Workaround.cpp
 
     PYTHON_CPPFILES
         moduleDeps.cpp