fix include order #1211
fix include order #1211
Conversation
| @@ -86,6 +84,8 @@ if(NOT DEFINED PXR_USD_LOCATION) | |||
| endif() | |||
| endif() | |||
|
|
|||
| include(${USD_CONFIG_FILE}) | |||
There was a problem hiding this comment.
I build MayaUsd by setting the Maya and Pixar locations with an env var so this tripped me up. The pxrConfig file needs PXR_USD_LOCATION to be set, but you copy/pasted that import line to just before the code that might set it based on the env var. Just moving the include down to after that and I'm all good now.
There was a problem hiding this comment.
Hmm. I'm not quite seeing how anything in pxrConfig.cmake depends on PXR_USD_LOCATION. As far as I'm aware PXR_USD_LOCATION as either a CMake variable or an environment variable is only understood by maya-usd, not core USD. But you're clearly hitting an issue with this, so I don't at all mean to discount that.
Following that logic though, if there is some dependence on the value of PXR_USD_LOCATION that I'm not catching, should this include line go below the next block as well, somewhere around line 94?
There was a problem hiding this comment.
I might have written that incorrectly. It's in pxrTargets.cmake, which is included from pxrConfig.cmake (if I've got it straight this time).
pxrTargets has: INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${PXR_USD_LOCATION}/include/boost-1_70"
There was a problem hiding this comment.
Oh, interesting. Yeah, I thought I was accounting for anything that might get included by pxrConfig.cmake.
That may have something to do with how core USD is getting built then. Are you specifying the path to Boost using PXR_USD_LOCATION when you build core USD? It would seem that it's injecting that path unexpanded into the pxrTargets.cmake.
For my builds, pxrTargets.cmake has the ${_IMPORT_PREFIX}/include path followed by absolute paths to the other dependencies, since I'm feeding absolute paths to the core USD build.
There was a problem hiding this comment.
I took a second look with Sean and we agreed that both of those chunks of logic probably aren't required, it would be most correct to just make sure that PXR_USD_LOCATION is set to wherever we found pxrConfig.cmake. Then if it was passed in through an env var, or it was possibly a hint-list, then it's always set correctly.
There was a problem hiding this comment.
I'd have to check with Sean about the USD build settings. I haven't built USD from source in a long time, only used the binaries they have provided us all.
There was a problem hiding this comment.
Added @seando-adsk so he sees your question about the USD build.
There was a problem hiding this comment.
@mattyjams When we build USD, we post-process the pxrTargets/pxrConfig files and replace the hard-coded paths with variables (one of them being PXR_USD_LOCATION). Then we zip this USD into what we call an artifact which our build machines download and use when building the plugin.
There was a problem hiding this comment.
Ahh, ok. I think I get it now. Such a tangled web we weave. :)
In any case, great that it was possible to simplify it down to what you have now.
There was a problem hiding this comment.
I don't quite follow the dependency on PXR_USD_LOCATION being set before pxrConfig.cmake gets included, but there's no issue with ensuring that that's the case, so seems ok to me.
I wonder though whether it should move one block further down if the value of PXR_USD_LOCATION really is important.
| @@ -86,6 +84,8 @@ if(NOT DEFINED PXR_USD_LOCATION) | |||
| endif() | |||
| endif() | |||
|
|
|||
| include(${USD_CONFIG_FILE}) | |||
There was a problem hiding this comment.
Hmm. I'm not quite seeing how anything in pxrConfig.cmake depends on PXR_USD_LOCATION. As far as I'm aware PXR_USD_LOCATION as either a CMake variable or an environment variable is only understood by maya-usd, not core USD. But you're clearly hitting an issue with this, so I don't at all mean to discount that.
Following that logic though, if there is some dependence on the value of PXR_USD_LOCATION that I'm not catching, should this include line go below the next block as well, somewhere around line 94?
| @@ -86,6 +84,8 @@ if(NOT DEFINED PXR_USD_LOCATION) | |||
| endif() | |||
| endif() | |||
|
|
|||
| include(${USD_CONFIG_FILE}) | |||
There was a problem hiding this comment.
Ahh, ok. I think I get it now. Such a tangled web we weave. :)
In any case, great that it was possible to simplify it down to what you have now.
kxl-adsk
added
the
ready-for-merge
No description provided.