Linux tests - Inject common LD_LIBRARY_PATHs into add_maya_test cmake function

[murphyeoin]

For our internal linux build, which doesn't use rpaths, we decided we'd like to remove runpath also from the DSO as we don't use it, and it seems best to keep things simple and more easily debuggable.

Traditionally we have relied on LD_LIBRARY_PATH to resolve library paths, and this allows us to keep doing that. To keep everyone else's setup simple, we only add to LD_LIBRARY_PATH if CMAKE_SKIP_RPATH is active.

Another way to do this would be like #409, where we provide an additional cmake var that can be populated by any build system. In this case we felt like the usage is probably standard enough that it might be usable by others. It also avoids having to update all (current and future) call sites with the extra logic.

[HamedSabri-adsk]

@murphyeoin I like the current approach. Lgtm! Thanks.

[pmolodo]

The basic idea here makes sense - if we're not adding to the RPATH, we'll need to add to LD_LIBRARY_PATH at test time.

Only thing that strikes me as odd is that so few things are added to LD_LIBRARY_PATH - only, some "core" paths, and one for AL. I would have expected to something within, ie, "plugin/pxr" and "plugin/adsk" as well. You've confirmed that a build of all the plugins, with CMAKE_SKIP_RPATH set, works with this change? For testing this, you might want to remove the addition of $ENV{LD_LIBRARY_PATH} - it's possible it works only because of some other (say-rez-based?) environment manipulation that's happening, outside of this project.

For that matter - are we sure we want to add $ENV{LD_LIBRARY_PATH}? It's true that we do similar things with some of the other environment variables - but you could make the case that we shouldn't be doing this for any of these, since it makes tests less reproducible in different environments.

[murphyeoin]

@elrond79 - at the moment we are only building and testing what we need to get AL_USDMaya working - so the core mayaUsd lib, the utils lib and then the AL_USDMayaPlugin itself.
We definitely rely on Rez adding things to the LD_LIBRARY_PATH - and I agree with you in principle that it's fairly much a wildcard where you can put anything you like, but with Rez I can't really see a way around it.... only adding to the LD_LIBRARY_PATH when CMAKE_SKIP_RPATH is active is my way of keeping my conscience clean here - i.e the change shouldn't affect those following the recommended and standard path

[pmolodo]

Ok, I can see how it would be essential to add $ENV{LD_LIBRARY_PATH} if NOTHING uses rpaths - including all of the upstream dependencies. Didn't think that through...

However - if we're adding to the LD_LIBRARY_PATH for one plugin, I think we should be doing it for all of them. If somebody else comes along, who also wants to avoid RPATHS, then tries to run the tests, they'll be very confused why all the AL tests pass and all the adsk/pxr ones fail. I don't think there should be that many paths to add - you can probably just find all ".so" in the install directory, (and filter out any plugins added via PXR_PLUGINPATH mechanics).

[murphyeoin]

Paul, I totally agree that that's the right thing to do, but it's going to be impossible for me to test until we have those plugins rezified, which we're not planning on doing anytime soon...and difficult for anyone else to test either. Would a "@todo: " with instructions on what needs to be added suffice for now? (Also happy to update https://github.com/Autodesk/maya-usd/blob/dev/doc/build.md with a section on RPATH/LD_LIBRARY_PATH and using "meta-build" systems in general?)

[kxl-adsk]

I'm not very excited about continuing to have changes that target only one plugin. Documenting such behavior in global build.md file will just increase confusion.

What I don't understand is why changes in other plugins would need to be much more complicated than in AL?

[ppt-adsk]

Hi @murphyeoin , you can certainly tell me really quick if I'm barking up the wrong tree, and clearly you know what you're doing for your setup, but there are things I don't understand (and maybe it's because my head isn't screwed in properly):

• rpath is not overridable with LD_LIBRARY_PATH, but runpath is, and I believe we're using runpath. Therefore, why do you need to change anything to start using LD_LIBRARY_PATH?
• When you write "we decided we'd like to remove runpath also from the DSO as we don't use it", this pull request is not doing that, or at least I don't understand how it's doing that.
• My understanding of this pull request is that it's inserting LD_LIBRARY_PATH into the test environment, if CMAKE_SKIP_RPATH is set, and that's all this pull request is about. Given the above, that runpath is overridable with LD_LIBRARY_PATH, if we still need to make cmake changes, can the changes be unconditional and not depend on CMAKE_SKIP_RPATH?
• With respect to adding this change everywhere, I am quite concerned about adding LD_LIBRARY_PATH wholesale, because this adds an element of variability (that @elrond79 mentions) and therefore an element for confusion and bug reports for a gain that I'm unsure of. Who would need this functionality for the Pixar and ADSK plugins? Why?

[pmolodo]

Paul, I totally agree that that's the right thing to do, but it's going to be impossible for me to test until we have those plugins rezified, which we're not planning on doing anytime soon...and difficult for anyone else to test either. Would a "@todo: " with instructions on what needs to be added suffice for now? (Also happy to update https://github.com/Autodesk/maya-usd/blob/dev/doc/build.md with a section on RPATH/LD_LIBRARY_PATH and using "meta-build" systems in general?)

I can see how, given the way you folks work, this change would certainly be helpful / useful for your needs. But, without changes, I just don't see this as having a large enough target audience - it's really only useful to those who are both using CMAKE_SKIP_RPATH AND are only building the AL plugin - for everyone else, it's either going to have no impact, be confusing, or, at worst, be counterproductive.

As an alternative suggestion / way forward - what if we just added a special cmake (or environment?) variable - say, MAYAUSD_USE_LD_LIBRARY_PATH_FOR_TESTS - that would make the exterior environment's $LD_LIBRARY_PATH be kept when running tests? You would need to make sure that the extra entries you're currently hardcoding in this PR would be in $ENV{LD_LIBRARY_PATH} before running the tests - but if you're already running your tests in a rez-environment, I'm sure there's a way to make that happen. (At worst, maybe add a dummy maya-usd-test package to the environ?)

That would at least be more general - would it potentially be a workable solution for you?

[murphyeoin]

Thanks everyone - as this proposal seems to be causing consternation and confusion, I'd like to propose a variant of what @elrond79 suggested above (and is suggested in the description of the PR at the top):
Another way to do this would be like #409, where we provide an additional cmake var that can be populated by any build system
Sounds like this approach was less controversial, and makes fewer assumptions about workflows...leaving more of the business logic up to AL's cmake system.
In this case, we would just add a

"LD_LIBRARY_PATH=${ADDITIONAL_LD_LIBRARY_PATHS}"

either in the test macro (as per current PR), or update the macro to handle LD_LIBRARY_PATH and then update the cmake files for the AL plugin only

...would that work for you @elrond79 @ppt-adsk @kxl-adsk ?

[pmolodo]

I'd be ok with that (either of those two approaches).

[kxl-adsk]

In this particular case, leaving more of the business logic up to AL's cmake system sounds good to me. The only thing I have no view on is the changes you are having in mind to the macro.

[ppt-adsk]

AL plugin only sounds good to me.

[murphyeoin]

@elrond79 @ppt-adsk @kxl-adsk attempt #2 - slightly different approach, hopefully as discussed

[HamedSabri-adsk]

Thank you @murphyeoin . It looks good to me.