clean up pr2

[HamedSabri-adsk]

This is a follow up PR to sabrih/MAYA-103365/clean_up_pr1.

What is the point of this PR?

I am going to try to explain this in steps and my hope is that it would make things more clear around these 2 questions:

#358 (comment)
#358 (comment)

Apologies in advance if it is too long.

Prerequisite:
A target has 2 requirements:

1- build requirements ( everything that is needed to **build** the target )
2- usage  requirements ( everything that is needed to **use** this target  as a dependency of another target)

Keywords definition:

`PRIVATE`: requirement should apply to just this target 
`PUBLIC `: requirement should apply to this target and anything that links to it
`INTERFACE`: requirement should apply just things that link to it

Having this information in mind, let's have a look at hdMaya and render/mayaToHydra targets since they are both good examples in the context of this post.

Let's start with hdMaya with current PR changes:

build/usage with heavy focus on header inclusion

To successfully build this target, we obviously need to know where the header files are:

    PRIVATE
        ${CMAKE_CURRENT_SOURCE_DIR}

Now by adding ${CMAKE_CURRENT_SOURCE_DIR}, cmake knows exactly all the about the tree structure under hdMaya project.

Inside the project, we can add the header files either by "" or <>. Either flavor works.
For sake of example, utils.cpp can include it's own header file like this:

#include "utils.h"  // I prefer this personally
or 
#include <utils.h>

utils.cpp can also include other header files inside the project like this:

#include "adapters/adapterDebugCodes.h"
#include "delegates/delegateRegistry.h"

Now, to successfully use this target, the dependencies obviously need to know where the header files are. This can be done by using INTERFACE. We promote the header files to the build directory and add it's location to the INTERFACE

    INTERFACE
        ${CMAKE_BINARY_DIR}/include

Doing so, targets who depend on hdMaya, do get the includes automatically.
For example for mtoh project to use hdMaya, all it needs to do is to declare it's dependency to this library.

target_link_libraries(${TARGET_NAME} 
    PRIVATE 
        hdMaya 
)

Here are some examples how source codes inside mtoh include the headers. again you can either use "" or "<>". Either flavor works.

defaultLightDelegate.cpp
#include <hdMaya/delegates/delegateDebugCodes.h>

renderOverride.cpp

#include <hdMaya/delegates/delegateRegistry.h>
#include <hdMaya/delegates/sceneDelegate.h>
#include <hdMaya/utils.h>

Hope you are not too bored by now.

[pmolodo]

Hi - so, what you've said mostly makes sense to me. The only thing that I think I want to clarify is the distinction between using "" vs <> for headers (and relative vs "absolute") paths.

For instance, you say that:

Inside the project, we can add the header files either by "" or <>. Either flavor works.
For sake of example, utils.cpp can include it's own header file like this:

#include "utils.h"  // I prefer this personally
or 
#include <utils.h>

utils.cpp can also include other header files inside the project like this:

#include "adapters/adapterDebugCodes.h"
#include "delegates/delegateRegistry.h"

And:

again you can either use "" or "<>". Either flavor works.

Now, everything you say here is correct - as far as it goes. Both flavors will, indeed, work.

However, there's an additional factor in play here, which is the style conventions which we've agreed on now, and are about to formally roll out. Those lay out specific rules for when are where "" vs <> should be used, and so, according to the style guidelines, "" and <> can not be used interchangeably. Specifically, the style guidelines state that <> (with absolute paths) must be used for public header files, and that "" should be used for private ones.

(A note on terminology - when I said public and private header files, I'm using the terms in a slightly different way than the PUBLIC, PRIVATE, and INTERFACE keywords cmake uses when specifying include directories. For the purposes of the style guidelines, public refers to headers which will be installed, and accessible to other projects linking against this project, while private refers to headers which are only needed and accessible when building this project. If we assume that all cmake files are properly modernized, then public would correspond to headers in directories cmake marks PUBLIC and INTERFACE, while private would only refer to headers in directories cmake marks as PRIVATE.)

That would mean that, for the headers you used in the examples above (which would all meet the definition of public headers), that these forms would be preferred:

#include <hdMaya/utils.h>
#include <hdMaya/adapters/adapterDebugCodes.h>
#include <hdMaya/delegates/delegateRegistry.h>

...while these forms would be discouraged:

#include "utils.h"                        # public headers use <>
#include <utils.h>                        # public headers use full absolute path
#include "hdMaya/utils.h"                 # public headers use <>
#include "adapters/adapterDebugCodes.h"   # public headers use <>
#include "delegates/delegateRegistry.h"   # public headers use <>
#include <delegates/delegateRegistry.h>   # public headers use full absolute path

Or, at least, that's how I'm interpreting the style guidelines... did you have a different interpretation?

[HamedSabri-adsk]

Those lay out specific rules for when are where "" vs <> should be used, and so, according to the style guidelines, "" and <> can not be used interchangeably.

Correct, my comment "we can add the header files either by "" or <>. Either flavor works" was more of a general information and I do agree with you that "" and <> can not be used interchangeably.
I am on board with what has been proposed in style guidelines regarding "" or <> usage.

I do agree with following style when used by projects who consume hdMaya.

#include <hdMaya/utils.h>
#include <hdMaya/adapters/adapterDebugCodes.h>
#include <hdMaya/delegates/delegateRegistry.h>

Which I have applied in this PR. Please see lib/render/mayaToHydra/renderOverride.cpp

My main source of confusion is applying the same concept inside the hdMaya project itself.

For instance, utils.cpp doesn't really need to include utils.h like this:

#include <hdMaya/utils.h>

instead of simply

#include "utils.h"

same thing to every single source codes inside hdMaya. For example, If utils.cpp wants to include something from inside it's own project, it should be able to do it this way

#include "adapters/adapterDebugCodes.h"
vs 
#include <hdMaya/adapters/adapterDebugCodes.h>

I would also like to avoid any use of relative path(../) in either CMake or C++ code.

I hope this makes sense. I would also like to see other's feedback in our TSC forum around this topic.

[pmolodo]

OK, will continue this discussion on the TSC forum if that's what you'd prefer...

[mattyjams]

Looks great to me! Just a few comment blocks in CMake files that I think can get removed.

I'm fine with angle braces everywhere as long as we're using full paths to promoted header files. Though the convention in the core USD code base is to use quotes and full paths for pxr headers, that is strictly a style choice. @kxl-adsk helped me recognize that with quotes the preprocessor is going to initially fail to find the header when it looks for it by appending the full path to the source file's directory. So it'll end up finding it the same way as if angle braces were used instead.

[mattyjams]

Should this comment block get deleted too?

[HamedSabri-adsk]

Ah, Thanks @mattyjams . Good point, please see 14218de

[mattyjams]

Delete this comment block?

[HamedSabri-adsk]

see 14218de

[mattyjams]

Delete this comment block?

[HamedSabri-adsk]

see 14218de

[mattyjams]

Delete this duplicated comment block?

[HamedSabri-adsk]

see 14218de

[pmolodo]

Other than header style stuff, which I see you're already addressing in #379, looks good!

[HamedSabri-adsk]

@mattyjams #379 (comment)