CMake Modernization and cleanup in lib and plugin/adsk #323

HamedSabri-adsk · 2020-03-03T17:23:35Z

Description:

This PR aims at making sure the CMake codes under lib and plugin/adsk are up to a good quality standard. As our code base grow, it is imperative to make sure there is consistency among CMake files and build/usage requirements of targets( shared libraries, executable, etc...) are very clear for the clients.

HamedSabri-adsk · 2020-03-03T17:38:16Z

CMakeLists.txt

 #
-
-cmake_minimum_required(VERSION 3.12.0)
+cmake_minimum_required(VERSION 3.13...3.17)


Setting Min and Max versions to be used.

One of the features introduced in version 3.13 is that target_sources doesn't have to be called in the same CMakeLists file that add_library is called.

If you look at lib/CMakeLists.txt, there is no traces of call to target_sources and rather this is delegated to each sub-directories (e.g ae, base, fileio, listeners, nodes, etc...).

If you are interested, there is a great article on the advantages of target_sources and enhancements in version 3.13:
https://crascit.com/2016/01/31/enhanced-source-file-handling-with-target_sources/

I may be missing something here, but why do we care about a max version if we're currently compatible with the most recent release? Seems like this would be something we'd need to update periodically to support newer releases of CMake as they come out.

I don't think we necessarily need to update this periodically or at least any times soon to support newer features.

The version range basically specifies that the current CMake code is written and tested for the given range of CMake versions and avoids having to setup the explicit policy settings.

"max" compatibility version affects policy settings. So, if someone is using a version which is newer than let's say 3.19 then they can set policies as far as the "max" version.

Please see this:
https://cmake.org/cmake/help/v3.12/command/cmake_minimum_required.html

Ahh, sorry. See? I should have rtfm. :)

I foolishly assumed the max was a hard cutoff on which versions of CMake the project could be built with. Makes a lot more sense that it only determines policy settings instead. Duh...

Lol, no worries.... I am no CMake expert myself at all and have to read the manuals from time to time to understand things :)

HamedSabri-adsk · 2020-03-03T17:38:57Z

CMakeLists.txt

-#==============================================================================
+#------------------------------------------------------------------------------
+# options
+#------------------------------------------------------------------------------


replacing = with -. It is less distracting.

HamedSabri-adsk · 2020-03-03T17:39:42Z

CMakeLists.txt

+  set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--enable-new-dtags")
+endif()
+
+#------------------------------------------------------------------------------


organizing global options.

HamedSabri-adsk · 2020-03-03T17:40:53Z

CMakeLists.txt

-#==============================================================================
-# Tests
-#==============================================================================
+#------------------------------------------------------------------------------


Leaving compiler configuration as is for now!! This will change in the second phase of cmake clean up.

HamedSabri-adsk · 2020-03-03T17:42:27Z

cmake/jinja.cmake

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#


Keeping the Copyright style consistence.

Also,we don't need to add Copyright headers in every Cmake files ( it is very distracting ). Only add in the top level cmake file.

HamedSabri-adsk · 2020-03-03T17:43:18Z

cmake/modules/FindMaya.cmake

@@ -33,7 +33,7 @@
 #  - OS-specific defines
 #  - Post-commnad for correcting Qt library linking on osx
 #  - Windows link flags for exporting initializePlugin/uninitializePlugin
-macro(MAYA_SET_PLUGIN_PROPERTIES target)
+macro(maya_set_plugin_properties target)


Keeping all function calls lowered cased for consistency.

HamedSabri-adsk · 2020-03-03T17:44:52Z

cmake/utils.cmake

 #
-#   SUBDIR        - optional sub-directory in which to promote files.
-#   FILES         - list of files to promote.


removed duplicated mayaUsd_promoteHeaderListWithSubdir and consolidated some of it's functionalities into mayaUsd_promoteHeaderList

HamedSabri-adsk · 2020-03-03T17:57:06Z

lib/CMakeLists.txt

 add_subdirectory(usd)
+add_subdirectory(utils)


One of the main problems that this PR addresses is to clean up this CMake file.

Every sub-directories under lib now gets it's own CMakeLists.txt

All the target_xxx calls explicitly declare properties such as PUBLIC, PRIVATE. This reduces the chance to introduce hidden dependencies.

pmolodo · 2020-03-03T22:27:35Z

Haven’t done a full review, but was comparing this PR to #321... and to be clear, before either of these PRs, there were 3 functions with similar names: mayaUsd_promoteHeaderList, and TWO identical copies of mayaUsd_promoteHeaderListWithSubdir.

PR #321 removed the second copy of mayaUsd_promoteHeaderListWithSubdir, but left the other two unchanged. This PR removes the first copy of mayaUsd_promoteHeaderListWithSubdir, and updates mayaUsd_promoteHeaderList, but leaves the second copy of mayaUsd_promoteHeaderListWithSubdir untouched. Is this intentional?

… my previous commit.

hamedsabri · 2020-03-04T00:45:48Z

Haven’t done a full review, but was comparing this PR to #321... and to be clear, before either of these PRs, there were 3 functions with similar names: mayaUsd_promoteHeaderList, and TWO identical copies of mayaUsd_promoteHeaderListWithSubdir.

PR #321 removed the second copy of mayaUsd_promoteHeaderListWithSubdir, but left the other two unchanged. This PR removes the first copy of mayaUsd_promoteHeaderListWithSubdir, and updates mayaUsd_promoteHeaderList, but leaves the second copy of mayaUsd_promoteHeaderListWithSubdir untouched. Is this intentional?

whoops!! not intentional indeed. Fixed in 056e830.

mattyjams

Thanks for tackling this @HamedSabri-adsk! It's looking way better already.

The one important thing I think needs to be looked at is the module.cpp/moduleDeps.cpp thing for mayaUsd, but otherwise this is looking pretty good. I'm no CMake expert, but I like the direction this is heading, and we can do additional cleanup in subsequent passes.

mattyjams · 2020-03-04T23:58:17Z

CMakeLists.txt

 #
-
-cmake_minimum_required(VERSION 3.12.0)
+cmake_minimum_required(VERSION 3.13...3.17)


I may be missing something here, but why do we care about a max version if we're currently compatible with the most recent release? Seems like this would be something we'd need to update periodically to support newer releases of CMake as they come out.

mattyjams · 2020-03-05T00:05:19Z

cmake/utils.cmake

 function(mayaUsd_promoteMayaUsdHeader)
-    set(srcFile ${CMAKE_CURRENT_SOURCE_DIR}/base/mayaUsd.h.src)
+    set(srcFile ${CMAKE_CURRENT_SOURCE_DIR}/mayaUsd.h.src)
    set(dstFile ${CMAKE_BINARY_DIR}/include/mayaUsd/mayaUsd.h)
    if (NOT EXISTS ${dstFile})
        message(STATUS "promoting: " ${srcFile})


I'm not really sure why there's a utility function for this. Seems like its contents should just go in the lib (or lib/base?) CMakeLists.txt?

I can understand why this could be confusing.

This utility basically calls cmake "configure_file" function to copy an input file to an output file and substitutes variable values referenced as @var@ or ${VAR} in the input file content.

input = mayaUsd.h.src output = include/mayaUsd/mayaUsd.h

The reason base/ is removed is because every sub-directories under "lib" is now getting it's own CMakeLists.txt and we no longer need to have hard-coded paths in utilities anymore.

Oh, sorry if I was unclear. I think I get what the function does. My gripe was more that I think as a function, this only gets called in one place, doesn't it? I would think we only install the mayaUsd.h file once. Can/should we just put this code directly in the lib/base/CMakeLists.txt?

Correct, this function is only called once for copying mayaUsd.h.src which is in lib/base/CMakeLists.txt

You have a point, this is now directly called in lib/base/CMakeLists.txt.

See 71d2a8b

mattyjams · 2020-03-05T00:07:53Z

lib/CMakeLists.txt

+        MAYAUSD_CORE_EXPORT
+        MFB_PACKAGE_NAME="${PROJECT_NAME}"
+        MFB_ALT_PACKAGE_NAME="${PROJECT_NAME}"
+        MFB_PACKAGE_MODULE=mayaUsd


${PROJECT_NAME} here too instead of the literal mayaUsd?

Good point. Will fix it in the next commit.

Please see e25fa48

mattyjams · 2020-03-05T00:10:13Z

lib/CMakeLists.txt

+target_include_directories(${PROJECT_NAME} 
+    PUBLIC
+        ${CMAKE_BINARY_DIR}/include
+        ${MAYA_INCLUDE_DIRS}
+        ${PXR_INCLUDE_DIRS}
+        $<$<BOOL:${UFE_FOUND}>:${UFE_INCLUDE_DIR}>
+    PRIVATE
+        ${CMAKE_CURRENT_SOURCE_DIR}
+)


Not sure if this is slated for a later phase, but it would be great to verify that all of the named targets we're using with target_link_libraries() properly identify their interface include directories so that we don't need to specify them here. I think the pxr libraries should be doing that already, and maybe we should be setting up Maya::* and UFE::* targets in the Find modules.

mattyjams · 2020-03-05T00:15:01Z

lib/CMakeLists.txt

+        ${Boost_FILESYSTEM_LIBRARY}
+        ${Boost_SYSTEM_LIBRARY}


Can we start using the named boost targets now, like Boost::filesystem and Boost::system?

Good idea. I like your suggestion. Will fix it in the next commit.

see e25fa48

mattyjams · 2020-03-05T00:26:59Z

lib/base/CMakeLists.txt

+mayaUsd_promoteMayaUsdHeader()
+mayaUsd_promoteHeaderList(HEADERS ${headers} SUBDIR base)
+
+# -----------------------------------------------------------------------------
+# install
+# -----------------------------------------------------------------------------
+install(FILES ${headers}
+    DESTINATION ${CMAKE_INSTALL_PREFIX}/include/mayaUsd/base
+)
+
+install(FILES ${CMAKE_BINARY_DIR}/include/mayaUsd/mayaUsd.h
+    DESTINATION ${CMAKE_INSTALL_PREFIX}/include/mayaUsd
+)


I may be missing something here too, but promoting and installing feels somewhat redundant to me, and I'm not crazy about how the promote function injects #pragma directives on the fly. Is there a way to collapse this? Or maybe that's also planned for a later pass?

Promoting and installing are kind of orthogonal to each other. The problem with collapsing them is that "install stage" doesn't necessarily need to happen all the time but the "build stage" does.

Also relying on install stage to access the header files doesn't really work. What happens with incremental builds? That's why we include headers from the build directory when building the project. Also when the incremental build happens ONLY the headers files that have touched are promoted.

Please let me know if this answers your question or perhaps I got your question confused with something else.

As I was reading your above comment in lib/CMakeLists.txt , I realized that having ${CMAKE_BINARY_DIR}/include in the PUBLIC list makes it a bit vague when other projects link against mayaUsd project.

This is now added in the PRIVATE entry list and projects that links against mayaUsd now need to explicitly add ${CMAKE_BINARY_DIR}/include in their target_include_directories.

mayaUsd

target_include_directories(${PROJECT_NAME} PUBLIC ${MAYA_INCLUDE_DIRS} ${PXR_INCLUDE_DIRS} $<$<BOOL:${UFE_FOUND}>:${UFE_INCLUDE_DIR}> PRIVATE ${CMAKE_BINARY_DIR}/include ${CMAKE_CURRENT_SOURCE_DIR} )

_mayaUsd

add_library(${PYTHON_TARGET_NAME} SHARED) target_include_directories(${PYTHON_TARGET_NAME} PRIVATE ${CMAKE_BINARY_DIR}/include ) target_link_libraries(${PYTHON_TARGET_NAME} PRIVATE mayaUsd )

please see 904b394

I may again be exposing my CMake ignorance, but is this not what the INTERFACE keyword is for?

It seems like things that link against mayaUsd shouldn't need to do anything special (like add an include directory) other than just declare their dependence on mayaUsd. The build and install requirements necessary for a dependency of a downstream library should all be determined by the properties of the upstream target.

I think I probably need to do my CMake homework here, and am totally happy to revisit this later when I have a better understanding.

Not at all.... You have a great point and I am glad we revisit this and change things.

My understating is that PRIVATE properties are used internally to build the target. Hence, PRIVATE models the build requirements of a target.

On the other hand, INTERFACE properties are used externally by users of the target. Hence, INTERFACE properties models the usage requirements of a target.

What initially confused me (and possibly what tripped @mattyjams up as well?), was this bit (emphasis mine):

This is now added in the PRIVATE entry list and projects that links against mayaUsd now need to explicitly add ${CMAKE_BINARY_DIR}/include in their target_include_directories.

On re-reading this thread, I now assume that you were speaking hypothetically. Is this correct? Or are there actually cases where an external project would want / need to do this?

Correct, my comment was mostly hypothetical. I am happy if we want to revisit this but let's do it after this PR merged.

mattyjams · 2020-03-05T00:43:13Z

lib/python/CMakeLists.txt

+        module.cpp
+        moduleDeps.cpp


I think there's a subtle distinction in purpose between these two files that's not represented here.

module.cpp is definitely intended to get built into the Python module library object (lib/python/mayaUsd/lib/_mayaUsd.so on Linux), but moduleDeps.cpp is intended to get built into the main library (lib/libmayaUsd.so on Linux).

For pxr libraries, module.cpp is identified as a PYMODULE_CPPFILES:
https://github.com/PixarAnimationStudios/USD/blob/40c813421425c47aa84f4572b4798fbb3430984e/cmake/macros/Public.cmake#L304
Whereas moduleDeps.cpp is identified as a PYTHON_CPPFILES:
https://github.com/PixarAnimationStudios/USD/blob/40c813421425c47aa84f4572b4798fbb3430984e/cmake/macros/Public.cmake#L219

The upshot is that I think moduleDeps.cpp need to go up a level and be included in the main lib/CMakeLists.txt

Ugh I didn't know. Will fix it in the next commit. Thank you for the links.

please see 12f14b6

mattyjams · 2020-03-05T00:45:20Z

cmake/utils.cmake

@@ -401,15 +321,17 @@ function(separate_argument_list listName)
 endfunction()

 # python extension module suffix
-function(set_python_module_suffix target)
+function(set_python_module_property target)


This new function name is a little mysterious and looks a bit weird in CMakeLists.txt files below. What property? :P

It's a bit verbose, but maybe set_python_module_suffix_property()? Or do we expect that other properties might need to be added here? If that's the case maybe set_python_module_properties()?

And/or possibly prefix this with mayaUsd_? I'd be mildly concerned that this could be mistaken for a built-in CMake function.

set_python_module_property is simply a tiny wrapper around cmake set_target_properties function that sets both prefix and suffix names for python modules on different platforms.

function(set_python_module_property target) if(IS_WINDOWS) set_target_properties(${target} PROPERTIES PREFIX "" SUFFIX ".pyd" ) elseif(IS_LINUX OR IS_MACOSX) set_target_properties(${target} PROPERTIES PREFIX "" SUFFIX ".so" ) endif() endfunction()

Every cmake target (shared library, execturable,...) has some properties that can be modified through set_target_properties function.

Here is the list of all the properties on targets:

https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#properties-on-targets

Besides Targets, (Tests, Directories, Source Files, Cache Entries) can also have their own properties which are all listed here:

https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#id1

I totally agree with the name I chose being off. :) What do you think about

mayaUsd_set_python_module_properties

Yup, I think it was mostly the name that tripped me up. mayaUsd_set_python_module_properties sounds great to me.

mattyjams · 2020-03-05T00:51:06Z

lib/render/mayaToHydra/CMakeLists.txt

+# -----------------------------------------------------------------------------
+target_include_directories(${TARGET_NAME} 
+    PRIVATE 
+        "${CMAKE_CURRENT_SOURCE_DIR}/.."


This relative pathing is kind of unfortunate. I'd rather see this use headers from the build or install tree.

Totally agree. I am not really a big fan either. This was the way the cmake code came from mayaToHydra project and I wanted to discuss it with Paul as well.

Yup, fine for now, but worth revisiting later.

Agreed as well!
Actually, my hope is that once everything is properly modernized, we'll inherit this directory "for free", from linking against hdMaya, and we can drop this line entirely.

- using Boost::filesystem and Boost::system instead of ${Boost_FILESYSTEM_LIBRARY} and ${Boost_SYSTEM_LIBRARY}

…ct and didn't really belong in the public list. This makes it more explicit and clear as where the include directories are coming from. - Removed unused ${MAYAUSD_INCLUDE_DIR} variable.

… the Python module library.

mattyjams

Looks good to me! I'm sure we'll refine this stuff more as we go, but this is a great step forward. Thanks @HamedSabri-adsk!

mattyjams · 2020-03-05T19:27:33Z

CMakeLists.txt

 #
-
-cmake_minimum_required(VERSION 3.12.0)
+cmake_minimum_required(VERSION 3.13...3.17)


Ahh, sorry. See? I should have rtfm. :)

I foolishly assumed the max was a hard cutoff on which versions of CMake the project could be built with. Makes a lot more sense that it only determines policy settings instead. Duh...

mattyjams · 2020-03-05T19:31:18Z

cmake/utils.cmake

 function(mayaUsd_promoteMayaUsdHeader)
-    set(srcFile ${CMAKE_CURRENT_SOURCE_DIR}/base/mayaUsd.h.src)
+    set(srcFile ${CMAKE_CURRENT_SOURCE_DIR}/mayaUsd.h.src)
    set(dstFile ${CMAKE_BINARY_DIR}/include/mayaUsd/mayaUsd.h)
    if (NOT EXISTS ${dstFile})
        message(STATUS "promoting: " ${srcFile})


Oh, sorry if I was unclear. I think I get what the function does. My gripe was more that I think as a function, this only gets called in one place, doesn't it? I would think we only install the mayaUsd.h file once. Can/should we just put this code directly in the lib/base/CMakeLists.txt?

mattyjams · 2020-03-05T19:32:48Z

cmake/utils.cmake

@@ -401,15 +321,17 @@ function(separate_argument_list listName)
 endfunction()

 # python extension module suffix
-function(set_python_module_suffix target)
+function(set_python_module_property target)


Yup, I think it was mostly the name that tripped me up. mayaUsd_set_python_module_properties sounds great to me.

mattyjams · 2020-03-05T19:46:06Z

lib/base/CMakeLists.txt

+mayaUsd_promoteMayaUsdHeader()
+mayaUsd_promoteHeaderList(HEADERS ${headers} SUBDIR base)
+
+# -----------------------------------------------------------------------------
+# install
+# -----------------------------------------------------------------------------
+install(FILES ${headers}
+    DESTINATION ${CMAKE_INSTALL_PREFIX}/include/mayaUsd/base
+)
+
+install(FILES ${CMAKE_BINARY_DIR}/include/mayaUsd/mayaUsd.h
+    DESTINATION ${CMAKE_INSTALL_PREFIX}/include/mayaUsd
+)


I may again be exposing my CMake ignorance, but is this not what the INTERFACE keyword is for?

It seems like things that link against mayaUsd shouldn't need to do anything special (like add an include directory) other than just declare their dependence on mayaUsd. The build and install requirements necessary for a dependency of a downstream library should all be determined by the properties of the upstream target.

I think I probably need to do my CMake homework here, and am totally happy to revisit this later when I have a better understanding.

mattyjams · 2020-03-05T19:47:27Z

lib/render/mayaToHydra/CMakeLists.txt

+# -----------------------------------------------------------------------------
+target_include_directories(${TARGET_NAME} 
+    PRIVATE 
+        "${CMAKE_CURRENT_SOURCE_DIR}/.."


Yup, fine for now, but worth revisiting later.

mattyjams · 2020-03-05T19:52:36Z

lib/usd/hdMaya/CMakeLists.txt

+        mayaUsd
+        ${OPENGL_gl_LIBRARY} 
+        ${MAYA_LIBRARIES}
+        ${Boost_LIBRARIES} 


Boost::boost here, maybe?

…his is not really a utility function. - replace ${Boost_LIBRARIES} with Boost::boost

HamedSabri-adsk · 2020-03-05T21:11:17Z

Looks good to me! I'm sure we'll refine this stuff more as we go, but this is a great step forward. Thanks @HamedSabri-adsk!

@mattyjams Thanks a lot for all the great feedback. You are a CMake Ninja yourself and don't show :) I am sure we can refine things as we go and more than happy to do so.

Cheers

…d_plugin

kxl-adsk

MacOS build is failing during linking of lib/libmayaUsd.dylib

…at I missed in the latest merge.

HamedSabri-adsk · 2020-03-09T19:37:26Z

MacOS build is failing during linking of lib/libmayaUsd.dylib

See 9187c51

pmolodo

Looks good to me - just a few small questions / comments!

cmake/utils.cmake

test/lib/CMakeLists.txt

pmolodo

When I tried to build with these changes, I got an error when linking DiffCore - it couldn't find pthread.

When I changed this line:

maya-usd/test/lib/usd/utils/CMakeLists.txt

Line 28 in 9187c51

${GTEST_LIBRARIES}

...to instead use the GTest::GTest target (and eliminated explicit add of ${GTEST_INCLUDE_DIRS} as well), then this error went away.

[184/400] Linking CXX executable test/lib/usd/utils/DiffCore
FAILED: test/lib/usd/utils/DiffCore 
: && /usr/lib64/ccache/g++  -msse3  -std=c++11 -Wall -Werror -Wno-deprecated -Wno-deprecated-declarations -Wno-unused-local-typedefs -D_GLIBCXX_USE_CXX11_ABI=0  -std=c++11 -Wall -Werror -Wno-deprecated -Wno-deprecated-declarations -Wno-unused-local-typedefs -O3 -DNDEBUG  -Wl,--enable-new-dtags test/lib/usd/utils/CMakeFiles/DiffCore.dir/main.cpp.o test/lib/usd/utils/CMakeFiles/DiffCore.dir/test_DiffCore.cpp.o  -o test/lib/usd/utils/DiffCore -L/luma/soft/rez/packages/tbb/2017.0.6.nolibstdcpp.1/platform-linux/lib -Wl,-rpath,/Volumes/sv-dev01/devRepo/paulm/projects/rez-build/maya_usd/dev/build/platform-linux/gcc-6.3/maya-2019/usd-0.20.02.luma02/tbb-2017.0/lib/usd/utils:/luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib:/usr/local/python-2.7.7/lib:/luma/soft/rez/packages/boost/1.61.0/platform-linux/gcc-6.3.1/python-2.7/lib:/luma/soft/rez/packages/tbb/2017.0.6.nolibstdcpp.1/platform-linux/lib:/luma/soft/rez/packages/jemalloc/4.4.0/platform-linux/gcc-6.3.1/lib /luma/soft/rez/packages/googletest/1.8.0.fPIC/platform-linux/lib64/libgtest.a lib/usd/utils/libmayaUsdUtils.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumausd.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumakind.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumapcp.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumasdf.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumaar.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumaplug.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumavt.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumagf.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumawork.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumatrace.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumajs.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumatf.so /luma/soft/rez/packages/usd/0.20.02.luma02/platform-linux/gcc-6.3/lib/liblumaarch.so -ldl /usr/lib64/libm.so /usr/local/python-2.7.7/lib/libpython2.7.so /luma/soft/rez/packages/boost/1.61.0/platform-linux/gcc-6.3.1/python-2.7/lib/libboost_python.so -ltbb /luma/soft/rez/packages/jemalloc/4.4.0/platform-linux/gcc-6.3.1/lib/libjemalloc.so && :
/opt/rh/devtoolset-6/root/usr/libexec/gcc/x86_64-redhat-linux/6.3.1/ld: /luma/soft/rez/packages/googletest/1.8.0.fPIC/platform-linux/lib64/libgtest.a(gtest-all.cc.o): undefined reference to symbol 'pthread_key_delete@@GLIBC_2.2.5'
//lib64/libpthread.so.0: error adding symbols: DSO missing from command line

- Remove unnecessary CopyRight

HamedSabri-adsk · 2020-03-10T01:40:49Z

DiffCore

Hmm, I don't understand the gtest issue. What do you get when you print out GTEST_INCLUDE_DIRS , GTEST_LIBRARIES ?

message("GTEST_INCLUDE_DIRS = " ${GTEST_INCLUDE_DIRS})
message("GTEST_LIBRARIES = " ${GTEST_LIBRARIES})

HamedSabri-adsk · 2020-03-10T02:00:49Z

FWIW, I don't see the gtest issue on either MacOS or Windows and DiffCore test passes fine. I will double check things on my Linux machine tomorrow.

HamedSabri-adsk · 2020-03-10T13:18:16Z

@elrond79 Fyi, I don't see the Gtest issue on Linux either. Are you using maya-usd/build.py script to build things? The reason I am asking is because I see some traces of rez in ^ comment.

pmolodo · 2020-03-10T14:02:42Z

Well, it may well be the case that there’s something different about my setup; but even if we were to assume there are no functional differences, doesn’t changing things to use the target instead of the explicit libs fit exactly within what this PR is designed to do - change cmake to use more modern, target based practices?

HamedSabri-adsk · 2020-03-10T16:41:44Z

@elrond79
I don't know how to interpret your comment. Is this towards the GTest issue you are facing or is this a general concern? It is still unclear to me why your build fails.
Let's focus on thing at the time. Yes, using target based practice is one of the main reasons we have this PR.

pmolodo · 2020-03-10T16:46:57Z

I'm saying let's ignore the particular GTest failure I got for a moment, and treat it as a general matter, regarding CMake modernization. Isn't changing

    ${GTEST_LIBRARIES}

for

    GTest::GTest

(and also removing the now unneeded ${GTEST_INCLUDE_DIRS}) a good change, just in that context?

HamedSabri-adsk · 2020-03-10T16:58:00Z

Ugh I see...... Yes, definitely.

In fact, I did a similar change for boost (Boost::boost vs ${Boost_LIBRARIES}) that Matt pointed out in this PR.

I already have in my Todo list to create a PR to fix things like ${MAYA_LIBRARIES}, ${TBB_LIBRARIES}, etc...

pmolodo · 2020-03-10T17:47:33Z

Cool - now, re: the specific error I got - I've now learned that error is NOT directly related to this PR, since I'm seeing the same thing on current dev. So the problem is some sort of intersection between one of the other recent changes merged into dev, and our particular compilation environment. (If I had to guess, it's something related to the details of how gtest is built...?)

However, since the fix makes sense in this PR's context, it would be nice to have it here. If you'd rather I make it into a separate PR, though, I'm ok with that too.

…NCLUDE_DIRS if using the target directly. This is done automatically.

HamedSabri-adsk · 2020-03-10T17:55:47Z

Cool - now, re: the specific error I got - I've now learned that error is NOT directly related to this PR, since I'm seeing the same thing on current dev. So the problem is some sort of intersection between one of the other recent changes merged into dev, and our particular compilation environment. (If I had to guess, it's something related to the details of how gtest is built...?)

However, since the fix makes sense in this PR's context, it would be nice to have it here. If you'd rather I make it into a separate PR, though, I'm ok with that too.

Consider it done: See 01122bb
As I mentioned in previous comment, I have a similar change that applies to Maya, TBB, etc... I will have a separate PR for that one.

pmolodo

Looks good, thanks!

HamedSabri-adsk · 2020-03-16T01:02:25Z

I just noticed switching to GTest::GTest has broken the debug build on Windows!! Something to look at tomorrow.

CMake Error at test/lib/usd/utils/CMakeLists.txt:3 (add_executable):
  Target "DiffCore" links to target "GTest::GTest" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

…smission attributes for hydra usd preview surface. (#323)

CMake Modernization and cleanup in lib and plugin/adsk

e3d4771

HamedSabri-adsk commented Mar 3, 2020

View reviewed changes

HamedSabri-adsk requested review from pmolodo and mattyjams March 3, 2020 18:00

HamedSabri-adsk mentioned this pull request Mar 3, 2020

[cmake] remove duplicate def of mayaUsd_promoteHeaderListWithSubdir #321

Closed

kxl-adsk added the build Related to building maya-usd repository label Mar 3, 2020

Remove mayaUsd_promoteHeaderListWithSubdir that I missed to remove in…

056e830

… my previous commit.

mattyjams suggested changes Mar 5, 2020

View reviewed changes

Hamed Sabri added 4 commits March 5, 2020 12:06

- Set MFB_PACKAGE_MODULE to "${PROJECT_NAME}

e25fa48

- using Boost::filesystem and Boost::system instead of ${Boost_FILESYSTEM_LIBRARY} and ${Boost_SYSTEM_LIBRARY}

- Add missing codes in lib/usd/ui that was caused by a bad merge.

e402ced

- ${CMAKE_BINARY_DIR}/include is now added privately in mayaUsd proje…

904b394

…ct and didn't really belong in the public list. This makes it more explicit and clear as where the include directories are coming from. - Removed unused ${MAYAUSD_INCLUDE_DIR} variable.

moduleDeps.cpp is intended to get built into the main library and not…

12f14b6

… the Python module library.

HamedSabri-adsk requested a review from mattyjams March 5, 2020 18:21

mattyjams approved these changes Mar 5, 2020

View reviewed changes

- remove mayaUsd_promoteMayaUsdHeader and call it's logic directly. T…

71d2a8b

…his is not really a utility function. - replace ${Boost_LIBRARIES} with Boost::boost

Merge branch 'dev' into sabrih/MAYA-103365/cmake_modernization_lib_an…

7fd9295

…d_plugin

kxl-adsk suggested changes Mar 9, 2020

View reviewed changes

Fix missing source codes for (UsdContextOps, UsdContextOpsHandler) th…

9187c51

…at I missed in the latest merge.

HamedSabri-adsk requested a review from kxl-adsk March 9, 2020 19:36

pmolodo approved these changes Mar 9, 2020

View reviewed changes

cmake/utils.cmake Outdated Show resolved Hide resolved

test/lib/CMakeLists.txt Show resolved Hide resolved

pmolodo suggested changes Mar 9, 2020

View reviewed changes

- BASEDIR is a one_value keyword

d470ae2

- Remove unnecessary CopyRight

Use GTest::GTest directly. There is really no need to include GTEST_I…

01122bb

…NCLUDE_DIRS if using the target directly. This is done automatically.

HamedSabri-adsk requested a review from pmolodo March 10, 2020 17:56

pmolodo approved these changes Mar 10, 2020

View reviewed changes

kxl-adsk approved these changes Mar 10, 2020

View reviewed changes

kxl-adsk merged commit 1bf461b into dev Mar 10, 2020

kxl-adsk deleted the sabrih/MAYA-103365/cmake_modernization_lib_and_plugin branch March 10, 2020 19:14

mattyjams mentioned this pull request Mar 12, 2020

build ufe Python bindings module consistently with other modules #209

Merged

ppt-adsk pushed a commit that referenced this pull request Feb 28, 2023

MAYA-126705 : Combine standard surface geometry opacity with the tran…

3011b68

…smission attributes for hydra usd preview surface. (#323)

		add_subdirectory(usd)
		add_subdirectory(utils)

CMake Modernization and cleanup in lib and plugin/adsk #323

CMake Modernization and cleanup in lib and plugin/adsk #323

Conversation

HamedSabri-adsk commented Mar 3, 2020

Description:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmolodo commented Mar 3, 2020

hamedsabri commented Mar 4, 2020

mattyjams left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmolodo Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyjams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk commented Mar 5, 2020

kxl-adsk left a comment

Choose a reason for hiding this comment

HamedSabri-adsk commented Mar 9, 2020

pmolodo left a comment

Choose a reason for hiding this comment

pmolodo left a comment

Choose a reason for hiding this comment

HamedSabri-adsk commented Mar 10, 2020

HamedSabri-adsk commented Mar 10, 2020

HamedSabri-adsk commented Mar 10, 2020

pmolodo commented Mar 10, 2020

HamedSabri-adsk commented Mar 10, 2020

pmolodo commented Mar 10, 2020

HamedSabri-adsk commented Mar 10, 2020 • edited Loading

pmolodo commented Mar 10, 2020

HamedSabri-adsk commented Mar 10, 2020

pmolodo left a comment

Choose a reason for hiding this comment

HamedSabri-adsk commented Mar 16, 2020

HamedSabri-adsk Mar 3, 2020 •

edited

Loading

mattyjams left a comment •

edited

Loading

pmolodo Mar 9, 2020 •

edited

Loading

HamedSabri-adsk commented Mar 10, 2020 •

edited

Loading