google / usd_from_gltf

Apache License 2.0
550 stars 86 forks source link

Failed linking on amazonlinux #5

Closed TyLindberg closed 5 years ago

TyLindberg commented 5 years ago

I was trying to build on an amazonlinux docker image and ran into an error when linking usd_from_gltf. It seems that libpng and libturbojpeg were installed to lib64 instead of lib, causing the following gmake error.

gmake[2]: *** No rule to make target `/root/build/lib/libpng16.a', needed by `usd_from_gltf/usd_from_gltf'

It seems that the error occurs here, where it is assumed that those two libraries are installed in lib.

https://github.com/google/usd_from_gltf/blob/ee170a00426779207333230a49c5786e3ce4b6b7/process/CMakeLists.txt#L63

C0DED00D commented 5 years ago

Just so I understand the issue correctly, if you were to change those lines in CMakeLists.txt to lib64, would the build complete successfully?

Thanks!

TyLindberg commented 5 years ago

This is a patch I wrote that allowed it to compile successfully on amazonlinux. It adds 64 to the end of the installation directory if CMake detects a 64-bit OS. I was assuming that it was related to 64-bit vs 32-bit OS differences, but since I haven't tested it out on any other docker images I'm uncertain if that's actually the case.

index 2aa118a..fa286f3 100644
--- a/process/CMakeLists.txt
+++ b/process/CMakeLists.txt
@@ -60,7 +60,14 @@ set_target_properties(process PROPERTIES PUBLIC_HEADER "${PROCESS_HEADERS}")
 # TODO: Libpng and libturbojpeg do not define library directories. We
 # install everything to <install_dir>/lib though, so we just use LIBRARY_DIR
 # from one of the other libraries.
-set(INSTALLED_LIBRARY_DIR ${stb_image_LIBRARY_DIR})
+if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+    # 64 bits
+    set(LIB_EXT "64")
+elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
+    # 32 bits
+    set(LIB_EXT "")
+endif()
+set(INSTALLED_LIBRARY_DIR "${stb_image_LIBRARY_DIR}${LIB_EXT}")

 if (MSVC)
 target_link_libraries(process
C0DED00D commented 5 years ago

I am on a 64-bit distro (Debian based) and it is installing to lib... :( We'll keep this on our radar and try to come up with a fix that works across all linuxes, or at least across Debian and AmazonLinux. :)

Thanks for bringing it to our attention!

TyLindberg commented 5 years ago

Sounds good! I'll try to install it on another RHEL based distro like CentOS and check if it exhibits the same behavior.

C0DED00D commented 5 years ago

This is totally untested, but I wonder if something like this would make sense: if(EXISTS ${stb_image_LIBRARY_DIR}/libpng16.a)) set(LIB_PNG_LOCATION, ${stb_image_LIBRARY_DIR}/libpng16.a)) else(EXISTS ${stb_image_LIBRARY_DIR}/libping16.a)) get_filename_component(PARENT_DIR ${stb_image_LIBRARY_DIR} DIRECTORY) set(LIB_PNG_LOCATION, ${PARENT_DIR}/lib64/libpng16.a) endif(EXISTS ${stb_image_LIBRARY_DIR}/libping16.a))

(Apologies for any syntax errors!)

What do you think? It feels a little hacky, but I'm not the best at CMake, and in my tests find_package(PNG REQUIRED) doesn't work for me, so I'm not sure if there is a better route.

TyLindberg commented 5 years ago

With some modifications I was able to make the snippet you sent work; however, it did feel a bit hacky so I did some looking around and came upon the find_library function which seems to fit our use case.

The following patch successfully compiles on amazonlinux for me.

diff --git a/process/CMakeLists.txt b/process/CMakeLists.txt
index 2aa118a..32af888 100644
--- a/process/CMakeLists.txt
+++ b/process/CMakeLists.txt
@@ -61,6 +61,15 @@ set_target_properties(process PROPERTIES PUBLIC_HEADER "${PROCESS_HEADERS}")
 # install everything to <install_dir>/lib though, so we just use LIBRARY_DIR
 # from one of the other libraries.
 set(INSTALLED_LIBRARY_DIR ${stb_image_LIBRARY_DIR})
+get_filename_component(PARENT_DIR ${stb_image_LIBRARY_DIR} DIRECTORY)
+find_library(PNG_LIBRARY
+  NAMES libpng16.a
+  HINTS ${stb_image_LIBRARY_DIR} ${PARENT_DIR}/lib64
+)
+find_library(TURBOJPEG_LIBRARY
+  NAMES libturbojpeg.a
+  HINTS ${stb_image_LIBRARY_DIR} ${PARENT_DIR}/lib64
+)

 if (MSVC)
 target_link_libraries(process
@@ -80,8 +89,8 @@ target_link_libraries(process
   "${draco_LIBRARY_DIR}/libdracodec.a"
   "${giflib_LIBRARY_DIR}/libgiflib.a"
   "${stb_image_LIBRARY_DIR}/libstb_image.a"
-  "${INSTALLED_LIBRARY_DIR}/libpng16.a"
-  "${INSTALLED_LIBRARY_DIR}/libturbojpeg.a"
+  "${PNG_LIBRARY}"
+  "${TURBOJPEG_LIBRARY}"
 )
 endif (MSVC)

It definitely needs some cleanup and error checking in case the libraries aren't found, but it seems to be a relatively robust solution and could even be extended to support MSVC as well if we specify the .lib files in the NAMES section.

Let me know your thoughts and if you have any luck with this solution on your end.

TyLindberg commented 5 years ago

Quick update on CentOS support. The same linking issue seems to occur; however, the patch I mentioned in my previous comment fixes it.

C0DED00D commented 5 years ago

Hello,

Thanks for all the help testing it and the suggestions. I just pushed a change that should work in your environments as well as mine. Can you give it a try?

Thanks!

TyLindberg commented 5 years ago

Compiles for me on amazonlinux and CentOS! Seems like we're good.

Thanks for getting that fix through!

C0DED00D commented 5 years ago

Glad to help! :)