cmake: fix for absolute path to libm.so in pxrConfig.cmake

[pmolodo]

Description of Change(s)

The absolute path makes pxrConfig.cmake non-portable between different distros of Linux (or any other situation where the location of libm.so differs).

I'm pushing two PRs with two different approaches to fixing this

• the "simple" fix (this one)
  • instead of linking to the absolute path of libm.so (found via find_library), we simply link to the lib m, and assume that the standard cmake variables / methods for finding libs will be sufficient
• the "safe" fix (PR2797)
  • we still use find_library to find the location of libm.so, but we don't link directly to the absolute path this finds; instead, we link to just m, but we make sure the directory we find it in is searched by the linker for any targets using it, via target_link_directories

Of the two approaches, I prefer the simple one (this one), because they will only differ in behavior if:

• someone is linking against a libm.so not installed in a standard system library directory (ie, not via the distro's standard package manager)
• AND this non-standard libm.sois in a location that WON'T be found at link time via CMake or the compiler's standard mechanisms for altering linking commands (ie, due to modifcations to the CMAKE_SHARED_LINKER_FLAGS cmake variable or the LDFLAGS env variable)
• AND this location WILL be found via find_library

I think that particular combination of circustances is rare enough that it's unlikely a current member of the USD community will see a difference. Given that, I'd prefer to keep the codebase simpler. (ie, the "safe" approach would require remembering to call target_link_directories(new_lib PRIVATE ${M_LIB_DIR}) on any new_lib which has a dependency on libm.so...)

Fixes Issue(s)

• pxrConfig.cmake generated on Linux not usable between distros due to absolute path of libm.so #2798

• I have verified that all unit tests pass with the proposed changes

• I have submitted a signed Contributor License Agreement

[jesschimein]

Filed as internal issue #USD-8922

[dgovil]

This version of your change is very much in line with my patch for macOS to fix the same issue.

#2772

I don't have a Linux box to test with but I think having the same format for linkage would be nice to have for uniformity.

[pmolodo]

This version of your change is very much in line with my patch for macOS to fix the same issue.

#2772

I don't have a Linux box to test with but I think having the same format for linkage would be nice to have for uniformity.

Hi @dgovil - I tested, and setting M_LIB to just m works on macOS as well. (I think this is slightly preferred to -lm, as it lets cmake adapt the flag to the toolchain if necessary. Also, it's more in keeping with how other libs are specified.)

Updated my PR just to remove the special casing for APPLE - in this case, anything non-windows can do it the same way.

[pmolodo]

On related note - I discovered the same basic issue exists for a number of other libs on Linux - here's the full list of Linux libs I had to patch up:

GL;GLU;ICE;SM;X11;Xext;Xt;m

Somewhat similarly, I also found some issues with full paths to static libs getting baked into the pxrTargets.cmake on windows:

osdCPU;osdGPU

In case anyone else has similar problems when trying to use pxrConfig.cmake, here's the cmake code I had to insert in my CMakeLists.txt, after the find_package, to fix it:

find_package(pxr
    HINTS
    ${USD_LOCATION}
    $ENV{USD_LOCATION}
    ${PXR_USD_LOCATION}
    $ENV{PXR_USD_LOCATION}
    ${PXR_DIR}
    $ENV{PXR_DIR}
)

# Fix for absolute paths to libs in pxrConfig.cmake - see:
# https://github.com/PixarAnimationStudios/OpenUSD/issues/2798
# https://github.com/PixarAnimationStudios/OpenUSD/pull/2796
function(fix_absolute_interface_linked_libs lib_names)
    cmake_parse_arguments(args
        "STATIC"
        ""
        ""
        ${ARGN}
    )

    if(args_STATIC)
        set(lib_prefix "${CMAKE_STATIC_LIBRARY_PREFIX}")
        set(lib_suffix "${CMAKE_STATIC_LIBRARY_SUFFIX}")
    else()
        set(lib_prefix "${CMAKE_SHARED_LIBRARY_PREFIX}")
        set(lib_suffix "${CMAKE_SHARED_LIBRARY_SUFFIX}")
    endif()

    string(LENGTH "${lib_suffix}." suffix_plus_dot_len)

    get_property(import_targets DIRECTORY "${CMAKE_SOURCE_DIR}" PROPERTY IMPORTED_TARGETS)

    foreach(target ${import_targets})
        get_target_property(interface_link_libraries ${target} INTERFACE_LINK_LIBRARIES)

        if(NOT interface_link_libraries)
            continue()
        endif()

        set(modified_interface_link_libraries, "")
        set(found_bad_lib FALSE)

        foreach(link_lib ${interface_link_libraries})
            list(APPEND modified_interface_link_libraries "${link_lib}")

            if(NOT IS_ABSOLUTE "${link_lib}")
                continue()
            endif()

            get_filename_component(link_lib_ext ${link_lib} EXT)

            set(is_lib_suffix FALSE)

            if("${link_lib_ext}" STREQUAL "${lib_suffix}")
                # check if the extension is exactly, ie, ".so"
                set(is_lib_suffix TRUE)
            elseif(NOT WIN32)
                # check if the extension starts with, ie, ".so." - like libm.so.6
                string(SUBSTRING "${link_lib_ext}" 0 ${suffix_plus_dot_len} possible_suffix_plus_dot)

                if("${possible_suffix_plus_dot}" STREQUAL "${lib_suffix}.")
                    set(is_lib_suffix TRUE)
                endif()
            endif()

            if(NOT is_lib_suffix)
                # if the extension isn't, ie, ".so" or ".so.6", skip
                continue()
            endif()

            if(EXISTS "${link_lib}")
                continue()
            endif()

            get_filename_component(link_lib_shortname ${link_lib} NAME_WE)

            foreach(lib_name ${lib_names})
                if("${link_lib_shortname}" STREQUAL "${CMAKE_SHARED_LIBRARY_PREFIX}${lib_name}")
                    # Ok, we found an absolute path to our lib, that doesn't exist on the current system
                    # replace with just the lib name
                    set(found_bad_lib TRUE)
                    message(VERBOSE "Fixing non-existant INTERFACE_LINK_LIBRARIES entry for '${target}': ${link_lib}")
                    list(POP_BACK modified_interface_link_libraries)

                    if(NOT args_STATIC)
                        list(APPEND modified_interface_link_libraries "${lib_name}")
                    endif()
                    break()
                endif()
            endforeach()

            if(found_bad_lib)
                set_target_properties(${target} PROPERTIES
                    INTERFACE_LINK_LIBRARIES
                    "${modified_interface_link_libraries}"
                )
            endif()
        endforeach()
    endforeach()
endfunction()

if(WIN32)
    fix_absolute_interface_linked_libs("osdCPU;osdGPU" STATIC)
else()
    fix_absolute_interface_linked_libs("GL;GLU;ICE;SM;X11;Xext;Xt;m")
endif()

[pmolodo]

As a follow up to this comment:

On related note - I discovered the same basic issue exists for a number of other libs on Linux - here's the full list of Linux libs I had to patch up:

GL;GLU;ICE;SM;X11;Xext;Xt;m

...I think most of the other absolute paths there came in "recursively" due to MaterialXConfig.cmake, and I've filed some PRs on their repo to fix these:

• Link to the OpenGL::GL target instead of OPENGL_LIBRARIES AcademySoftwareFoundation/MaterialX#1603
• Link to X11 targets instead of CMake variables AcademySoftwareFoundation/MaterialX#1606

Until this is fixed there, however, and we update to that version, we may want to consider applying these as patches to our source. I'll open a separate issue / PR for that, though, when/if I get the time...

[pmolodo]

...I think most of the other absolute paths there came in "recursively" due to MaterialXConfig.cmake, and I've filed some PRs on their repo to fix these:

• Link to the OpenGL::GL target instead of OPENGL_LIBRARIES AcademySoftwareFoundation/MaterialX#1603
• Link to X11 targets instead of CMake variables AcademySoftwareFoundation/MaterialX#1606

Just a quick note that the above two MRs were merged into MaterialX 1.38.9.

[pmolodo]

Hi - someone else ran into this issue, and was wondering if we might get this merged. Any chance it can make it in the next release?