The aim of this PR is to allow compiler flags to be easily and clearly added within Maya USD build echo system. #412

HamedSabri-adsk · 2020-04-08T19:30:23Z

PR Goal

The aim of this PR is to allow compiler flags to be easily and clearly added within Maya USD build echo system using modern cmake.

Background

When we originally started the Maya USD project, I brought in Pixar's compiler configuration( a.k.a defaults ) under the top level cmake to consume it's configurations.

This was done for two reasons:
1- It already worked out of the box when we migrated third-party/maya and it was convenient to use them for the sake of saving time.
2- other projects ( e.g Animal Logic ) was also consuming the exact same compiler configuration.

Problems

1- Pixar's compiler flags/options have a global affect and setting/appending to CMAKE_CXX_FLAGS is no longer a good practice ( see CMake documentation around this ). Everytime a compiler flag/option is added/removed/modified, it affects the entire project and literally everyone (Animal Logic , mayaUsd, mayaUsdPlugin, pxrusd).

2- Many of the compiler flags/options that came with defaults are either redundant or not needed at all which are carried over from core usd and old third-party/maya.

3- Hard to fine tune compiler options/flags around specific targets.

Solution

Introducing compiler_config.cmake where all the compiler flags/options are applied to a specific cmake echo system ( e.g mayaUsd and mayaUsdPlugin ). We use modern cmake functions ( target_compile_features, target_compile_options, target_compile_definitions) to control the options and flags. All of the compiler flags for Gnu, Clang, VS are picked as needed and are
privately added via mayaUsd_compile_config function.

Where is cmake/defaults?

cmake/defaults is moved to it's original place under plugin/pxr/cmake/defaults. I also made a small surgery in top-level AL's cmakelist to continue consuming pxr/cmake/defaults as before.

…y added within Maya USD build echo system.

HamedSabri-adsk · 2020-04-08T19:37:08Z

cmake/compiler_config.cmake

+
+set(msvc_flags
+    # we want to be as strict as possible
+    /W3


We currently have to use level 3 (/W3) instead of level 4(/W4) for Visual studio compiler since Pixar's USD core needs to adopt their API library to respect this level. Switching to /W4 is out of commission at the moment.

More on /W4 here:
https://docs.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level?view=vs-2019

HamedSabri-adsk · 2020-04-08T19:37:35Z

cmake/compiler_config.cmake

+    target_compile_features(${TARGET} 
+        PRIVATE
+            $<$<VERSION_LESS:MAYA_APP_VERSION,2019>:cxx_std_11>
+            $<$<VERSION_GREATER_EQUAL:MAYA_APP_VERSION,2019>:cxx_std_14>


Maya2018 ---> c++11
Maya2019 and above ---> c++14

HamedSabri-adsk · 2020-04-08T19:42:18Z

plugin/pxr/cmake/macros/Private.cmake

@@ -13,7 +13,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-include(Version)


This is redundant. It is already added at the top level cmake.

mattyjams

Mostly looks good to me, but it looks like no C++ language standard is being specified for the AL and pxr plugin code? If we combine this PR with #395, it seems like that would be addressed.

cmake/compiler_config.cmake

lib/mayaUsd/render/mayaToHydra/CMakeLists.txt

mattyjams · 2020-04-14T21:36:44Z

cmake/compiler_config.cmake

+            $<$<VERSION_LESS:MAYA_APP_VERSION,2019>:cxx_std_11>
+            $<$<VERSION_GREATER_EQUAL:MAYA_APP_VERSION,2019>:cxx_std_14>


I'm a little conflicted on burying the C++ language standard inside mayaUsd_compile_config(). It feels like this should be more of a repo-level setting. Do we envision having different parts of the repo on different standards? That would seem a bit weird to me.

And somewhat related to that, which parts of the tree do we expect would not call mayaUsd_compile_config() for their targets? For now, maybe that's the AL and pxr plugins, but do we expect that to be the case long term? I'm hoping to make some time in the not too distant future to remove all the pxr build stuff from the remaining pxr plugin code.

I'm a little conflicted on burying the C++ language standard inside mayaUsd_compile_config().

There are two ways of adding C++ standard in modern CMake.

1- Setting the C++ standard directly before any targets being defined which is the approach you have in your PR:

set(CMAKE_CXX_STANDARD 11) set(CMAKE_CXX_STANDARD_REQUIRED ON)

This setting applies to all targets globally which in this case is Ok because we obviously want all targets to use the exact same standard.

2- Setting the C++ standard based on features which is a more finer grained approach

target_compile_features(foo PRIVATE|PUBLIC|INTERFACE cxx_std_11)

Now, there reason I went with this approach is because mayaUsd_compile_config already use target_compile_options and target_compile_definitions and target_compile_features seems like a good fit there.

To be clear, every targets must add mayaUsd_compile_config. Notice that all the entries for target_compile_options, target_compile_definitions, target_compile_features are declared as PRIVATE which is very important. Any change in mayaUsd_compile_config affects all targets that are inheriting this function which simulates a global behavior.

It feels like this should be more of a repo-level setting. Do we envision having different parts of the repo on different standards? That would seem a bit weird to me.

Not at all, all targets should have the same standard.

And somewhat related to that, which parts of the tree do we expect would not call mayaUsd_compile_config() for their targets?

Every targets must add mayaUsd_compile_config

Right. I guess I was reacting to two things:

The fact that we're already breaking the "every target must add mayaUsd_compile_config()" rule, since the AL and pxr plugin code do not use it. But that's probably a whole other can of beans, and they probably should use it eventually.

But more generally, we're saying that every target must add mayaUsd_compile_config() so that it picks up (effectively) global settings. It seems a little counter-intuitive to me to require authors to do something in their CMakeLists.txt so that they pick up the settings all targets need?

Anyway, it doesn't bother me that much, and I'm likely missing other context here. Just wanted to raise the question. Thanks for humoring me!

mattyjams · 2020-04-14T21:43:24Z

lib/mayaUsd/CMakeLists.txt

-target_compile_definitions(${PROJECT_NAME} 
+target_compile_definitions(${PROJECT_NAME}
+    PUBLIC
+        USD_VERSION_NUM=${USD_VERSION_NUM}


This seems ok for now, but since it comes from a dependency, maybe it should be on the upstream libraries to ensure this is set on the imported target with INTERFACE_COMPILE_DEFINITIONS?

plugin/al/CMakeLists.txt

plugin/pxr/CMakeLists.txt

…ies to ALL targets in plugin/pxr and plugin/al.

mattyjams

Looks good to me! Thanks @HamedSabri-adsk!

There might be an issue building for Maya 2018 against core USD 20.05+? We don't build for 2018 anymore, and I'm not sure how much longer you all are intending to support 2018 either.

lib/mayaUsd/render/mayaToHydra/CMakeLists.txt

mattyjams · 2020-04-15T17:19:32Z

cmake/compiler_config.cmake

+            $<$<VERSION_LESS:MAYA_APP_VERSION,2019>:cxx_std_11>
+            $<$<VERSION_GREATER_EQUAL:MAYA_APP_VERSION,2019>:cxx_std_14>


Right. I guess I was reacting to two things:

The fact that we're already breaking the "every target must add mayaUsd_compile_config()" rule, since the AL and pxr plugin code do not use it. But that's probably a whole other can of beans, and they probably should use it eventually.

But more generally, we're saying that every target must add mayaUsd_compile_config() so that it picks up (effectively) global settings. It seems a little counter-intuitive to me to require authors to do something in their CMakeLists.txt so that they pick up the settings all targets need?

Anyway, it doesn't bother me that much, and I'm likely missing other context here. Just wanted to raise the question. Thanks for humoring me!

plugin/pxr/CMakeLists.txt

mattyjams · 2020-04-15T17:26:56Z

plugin/pxr/cmake/defaults/CXXDefaults.cmake

+if(${MAYA_APP_VERSION} STRLESS_EQUAL "2019")
+    set(CMAKE_CXX_STANDARD 11)
+elseif(${MAYA_APP_VERSION} STRGREATER_EQUAL "2019")
+    set(CMAKE_CXX_STANDARD 14)
+endif()


Unfortunately, I think the version of USD factors into this decision as well. USD 20.05 and later will require C++14, so for example I think attempting to build maya-usd for Maya 2018 against USD 20.05 would fail with this?

HamedSabri-adsk · 2020-04-16T01:48:51Z

lib/usd/ui/CMakeLists.txt

        $<$<BOOL:${IS_MACOSX}>:OSMac_>
+        $<$<BOOL:${IS_LINUX}>:LINUX>
+


I have noticed a lot of targets are adding these two flags privately. Since these flags are needed for Maya, it makes more sense I make them as part of PUBLIC section in mayaUsd shared library. I will have a separate PR to address this but for now it should be ok.

…was added recently and is only defined when building against UFE 0.2.x.

HamedSabri-adsk · 2020-04-16T03:41:56Z

lib/mayaUsd/CMakeLists.txt

+        PUBLIC
+            UFE_PREVIEW_VERSION_NUM=${UFE_PREVIEW_VERSION_NUM}
+    )
+endif()


Fix 2019 build issue on Windows ( Fatal Error C1017 invalid integer constant expression ).

UFE_PREVIEW_VERSION_NUM is a new variable that was added recently and only defined when building against UFE 0.x.x.

…aya version.

…ausd

- better if condition for switching between c++11 and c++14

…ausd

…options are either unused or have been OFF for ever.

HamedSabri-adsk · 2020-04-19T23:53:25Z

Hi @mattyjams,

I have updated this PR to respect the recent changes in dev branch. Following this, I decided to finish what we started in here and get plugin/pxr and plugin/al to adopt compiler configuration ( mayaUsd_compile_config) to avoid duplication and future confusions. The work is done in these 2 PRs( PR #443, PR #445 ).

Union of PR 412, 443, 445 makes this PR complete. Also defaults folders, is no longer in use and I didn't delete it until your confirmation.

mattyjams

Looks good to me! Thanks @HamedSabri-adsk!

mattyjams · 2020-04-21T01:49:23Z

doc/build.md

@@ -110,6 +110,8 @@ BUILD_PXR_PLUGIN            | builds the Pixar USD plugin and libraries.
 BUILD_AL_PLUGIN             | builds the Animal Logic USD plugin and libraries.          | ON
 BUILD_HDMAYA                | builds the Maya-To-Hydra plugin and scene delegate.        | ON
 BUILD_TESTS                 | builds all unit tests.                                     | ON
+BUILD_STRICT_MODE           | enforces all warnings as errors.                           | ON
+BUILD_SHARED_LIBS			| build libraries as shared or static.						 | ON


Funky whitespacing. Tab characters, maybe?

Hey @mattyjams , indeed funky whitespacing but one thing I have noticed is that is how github shows it when it wraps charcaters. In my text editor, I usually have word wrap off when I work with *.md file.

I have also noticed that sometimes github doesn't properly visualize spaces vs tabs.

That is because Github uses a default tab size of 8. I have installed this chrome extension to allow me to set it back to the standard size of 4. https://chrome.google.com/webstore/detail/github-custom-tab-size/jcjfkmdkcaopkioccnpbhiemfcmpnghe

That is because Github uses a default tab size of 8. I have installed this chrome extension to allow me to set it back to the standard size of 4. https://chrome.google.com/webstore/detail/github-custom-tab-size/jcjfkmdkcaopkioccnpbhiemfcmpnghe

Ughhhh... Thanks @seando-adsk. I didn't know that. I never use Chrome but try to see if they have a similar extension for Firefox.

…figuration_plugin_pxr plugin/pxr is now adopting mayaUsd_compile_config routine

* HYDRA-192 Remove WANT_UFE_BUILD option Remove the build flags that allowed UFE to be turned off. Make UFE a required library for the build. Remove some UFE-related tests in FindMaya.cmake that were based on old Maya versions that we don't support.

The aim of this PR is to allow compiler flags to be easily and clearl…

c95fcf2

…y added within Maya USD build echo system.

HamedSabri-adsk added the build Related to building maya-usd repository label Apr 8, 2020

HamedSabri-adsk requested a review from mattyjams April 8, 2020 19:30

HamedSabri-adsk commented Apr 8, 2020

View reviewed changes

kxl-adsk mentioned this pull request Apr 8, 2020

upgrade maya-usd project to the C++14 standard #395

Closed

mattyjams suggested changes Apr 14, 2020

View reviewed changes

Hamed Sabri added 2 commits April 15, 2020 10:32

Set required C++ version based on the Maya version. This setting appl…

dc898ce

…ies to ALL targets in plugin/pxr and plugin/al.

Fix a typo!

20ea6b7

HamedSabri-adsk requested a review from mattyjams April 15, 2020 14:36

mattyjams approved these changes Apr 15, 2020

View reviewed changes

HamedSabri-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label Apr 15, 2020

Hamed Sabri added 2 commits April 15, 2020 18:38

Fix debug build issues on Windows.

d9ec7c1

Fix Linux build!

71957d4

HamedSabri-adsk commented Apr 16, 2020

View reviewed changes

Fix 2019 build issue. UFE_PREVIEW_VERSION_NUM is a new variable that …

42bec8b

…was added recently and is only defined when building against UFE 0.2.x.

HamedSabri-adsk commented Apr 16, 2020

View reviewed changes

HamedSabri-adsk mentioned this pull request Apr 16, 2020

upgrade maya-usd project to the C++14 standard #438

Merged

Hamed Sabri added 4 commits April 16, 2020 18:13

Simplify the logics to switch between C++11 and C++14 dependeing on M…

b01a4a4

…aya version.

Merge branch 'dev' into sabrih/MAYA-103926/compiler_configuration_may…

602767d

…ausd

- make sure BUILD_SHARED_LIBS is ON

a58b023

- better if condition for switching between c++11 and c++14

Fix build issues on Windows against USD 0.20.5

9cc329b

HamedSabri-adsk mentioned this pull request Apr 19, 2020

plugin/pxr is now adopting mayaUsd_compile_config routine #442

Closed

Hamed Sabri added 2 commits April 19, 2020 13:09

Merge branch 'dev' into sabrih/MAYA-103926/compiler_configuration_may…

9437400

…ausd

plugin/pxr is now adopting mayaUsd_compile_config!

6502fed

This was referenced Apr 19, 2020

plugin/pxr is now adopting mayaUsd_compile_config routine #443

Merged

plugin/al is now adopting mayaUsd_compile_config routine #444

Closed

plugin/al is now adopting mayaUsd_compile_config routine #445

Merged

Fix PF build: plugin/pxr needs these 3 option flags. The rest of the …

fd7bcb7

…options are either unused or have been OFF for ever.

Fix Linux build!

7895ee9

mattyjams approved these changes Apr 21, 2020

View reviewed changes

Merge pull request #443 from Autodesk/sabrih/MAYA-103926/compiler_con…

b0c1441

…figuration_plugin_pxr plugin/pxr is now adopting mayaUsd_compile_config routine

HamedSabri-adsk removed the do-not-merge-yet Development is not finished, PR not ready for merge label Apr 21, 2020

kxl-adsk merged commit 09ae6ef into dev Apr 21, 2020

kxl-adsk deleted the sabrih/MAYA-103926/compiler_configuration_mayausd branch April 21, 2020 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The aim of this PR is to allow compiler flags to be easily and clearly added within Maya USD build echo system. #412

The aim of this PR is to allow compiler flags to be easily and clearly added within Maya USD build echo system. #412

HamedSabri-adsk commented Apr 8, 2020

HamedSabri-adsk Apr 8, 2020

HamedSabri-adsk Apr 8, 2020

HamedSabri-adsk Apr 8, 2020

mattyjams left a comment

mattyjams Apr 14, 2020

HamedSabri-adsk Apr 15, 2020

mattyjams Apr 15, 2020

mattyjams Apr 14, 2020

mattyjams left a comment

mattyjams Apr 15, 2020

mattyjams Apr 15, 2020

HamedSabri-adsk Apr 16, 2020

HamedSabri-adsk Apr 16, 2020

HamedSabri-adsk commented Apr 19, 2020

mattyjams left a comment

mattyjams Apr 21, 2020

HamedSabri-adsk Apr 21, 2020

seando-adsk Apr 21, 2020

HamedSabri-adsk Apr 21, 2020

		$<$<VERSION_LESS:MAYA_APP_VERSION,2019>:cxx_std_11>
		$<$<VERSION_GREATER_EQUAL:MAYA_APP_VERSION,2019>:cxx_std_14>

The aim of this PR is to allow compiler flags to be easily and clearly added within Maya USD build echo system. #412

The aim of this PR is to allow compiler flags to be easily and clearly added within Maya USD build echo system. #412

Conversation

HamedSabri-adsk commented Apr 8, 2020

PR Goal

Background

Problems

Solution

Where is cmake/defaults?

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

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

HamedSabri-adsk commented Apr 19, 2020

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