Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor cmake clean up pxr plugin #653

Merged
merged 5 commits into from
Jul 16, 2020
Merged

Conversation

HamedSabri-adsk
Copy link
Contributor

No description provided.

@HamedSabri-adsk HamedSabri-adsk added the build Related to building maya-usd repository label Jul 12, 2020
@@ -26,7 +25,6 @@ pxr_shared_library(${PXR_PACKAGE}
mayaUsd

INCLUDE_DIRS
${Boost_INCLUDE_DIRS}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these variables are empty?? The mystery part is how usdMaya links against Boost?

Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some cleanup is definitely warranted here since so much has moved out of these components, but I don't really agree with stripping it down to just mayaUsd. That may be part of what creates mysteries about where link dependencies are coming from when they come in transitively.

If an upstream library is included and used directly by a downstream library, the downstream library should explicitly declare that dependency.

@HamedSabri-adsk
Copy link
Contributor Author

If an upstream library is included and used directly by a downstream library, the downstream library should explicitly declare that dependency.

Respectfully, I have a different view on this. Explicitly declaring upstream library again in the downstream library is totally redundant. If upstream library is properly using PRIVATE/PUBLIC/INTERFACE keywords, one can look up the upstream dependencies to find out what they are.

For example, mayaUsdUtils is declaring gf, usd, sdf as PUBLIC. The downstream library DiffCore doesn't really need to repeat them again.

mayaUsdUtils

DiffCore

I am happy to revert the change if you still disagree. The important change in this PR is getting rid of those empty boost entries. They are very confusing and Sean and I were wondering how usdMaya builds without them. That's the real mystery :)

@mattyjams
Copy link
Contributor

Fair enough. I definitely see your point, and if the only reason that we were declaring that usdMaya has a dependency on vt, for example, was strictly because some other library that usdMaya depends on (mayaUsd in this case) is dependent on vt, then I completely agree with you. In that case the re-declaration is entirely redundant and should be removed.

In the case here though, usdMaya itself directly uses symbols from many of the libraries being removed by this change. So to me, it becomes an extension of the "include what you use" principle, or I guess now "link what you use".

Consider the case where in some future change, for whatever reason we decide to remove all usage of vt from mayaUsd and we remove vt as a dependency for mayaUsd. usdMaya still has #include directives for and its own usage of vt, so building usdMaya would suddenly break because of an otherwise inconsequential change in an upstream library. We would actually need to add vt as a dependency to usdMaya to fix that. Making sure we declare that direct dependency, even though we would otherwise pick it up transitively from some other library, helps make the build more robust against such changes in linkages.

So I'd still contend that many of these declared dependencies should remain here (basically any library that is #include'd by a code file in usdMaya should appear in the list of LIBRARIES in its CMakeLists.txt. vt is indeed still being used, but plug no longer is, so that would be a good candidate for removal.

Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as reviewed...

@HamedSabri-adsk
Copy link
Contributor Author

HamedSabri-adsk commented Jul 15, 2020

Consider the case where in some future change, for whatever reason we decide to remove all usage of vt from mayaUsd and we remove vt as a dependency for mayaUsd. usdMaya still has #include directives for and its own usage of vt, so building usdMaya would suddenly break because of an otherwise inconsequential change in an upstream library. We would actually need to add vt as a dependency to usdMaya to fix that. Making sure we declare that direct dependency, even though we would otherwise pick it up transitively from some other library, helps make the build more robust against such changes in linkages.

@mattyjams Thanks for the explanation. It makes sense to me now.

I removed plug as you suggested but put back everything else. See 114c4ec

Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks @HamedSabri-adsk! A few other now unused libraries could get yanked if you want, but otherwise looks good to me!

Comment on lines 6 to 11
gf
js
kind
plug
sdf
tf
usd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of the libraries still listed here, it looks like you could now also remove these if you wanted to:

js
usdLux
usdRi
usdShade
usdSkel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Please see f70e798

@kxl-adsk kxl-adsk merged commit 7e14067 into dev Jul 16, 2020
@kxl-adsk kxl-adsk deleted the sabrih/minor_clean_up_pixar_plugin branch July 16, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to building maya-usd repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants