Fixing macOS linking bug

[dhruvbatra]

Motivation and Context

Build from source on macOS fails at the linking step:
"ld: library not found for -lminizip"

Searching online the problem seems to be homebrew's cmake since Mojave:
https://stackoverflow.com/questions/54068035/linking-not-working-in-homebrews-cmake-since-mojave

How Has This Been Tested

By localing building on my mac.

Types of changes

• Docs change / refactoring / dependency upgrade
• [x ] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [ x] My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have completed my CLA (see CONTRIBUTING)
• I have added tests to cover my changes.
• All new and existing tests passed.

[erikwijmans]

Interesting. Can you try building with pip (or conda's) cmake, pip install cmake? If this is isolated to just homebrew it may be worth pointing people away from that.

@mosra is there any reason to not add a link like this? It scares me but I'm not sure if that's rational or not :)

[mosra]

The root cause is somewhere else and fixing it this way feels like a hack, sorry :) I'm not sure which dependency uses minizip (possibly Assimp?), but the proper workflow (instead of doing -lminizip or target_link_libraries(foo minizip) and expecting it to "just work") is:

1. find the actual library file on the filesystem using find_package() or find_library()
2. then call target_link_libraries() with the whole path to that file, not just minizip
3. which then makes CMake add proper link directory, if it's not among the implicit defaults and if it's not already added by something else

This however makes me wonder -- it basically means that habitat implicitly depends on system-installed minizip? What if the user just doesn't have that installed? (I probably don't.) There should be some way to make that dependency optional, bundle it or just disable the relevant functionality, since I'm not aware about any dataset format we use that would need to unzip anything.

I'll investigate which library is responsible for this and how it can be fixed directly there.

[mosra]

Yep, it's Assimp 🙄 The following patch should tell it to not look for minizip on the system but instead compile a bundled copy, which is what we want:

diff --git a/src/cmake/dependencies.cmake b/src/cmake/dependencies.cmake
index 8e5bad96..ff97b3e0 100644
--- a/src/cmake/dependencies.cmake
+++ b/src/cmake/dependencies.cmake
@@ -69,6 +69,11 @@ if(BUILD_ASSIMP_SUPPORT)
     # prefixed with ASSIMP_, so better set both variants to future-proof this.
     set(INJECT_DEBUG_POSTFIX OFF CACHE BOOL "" FORCE)
     set(ASSIMP_INJECT_DEBUG_POSTFIX OFF CACHE BOOL "" FORCE)
+    # Otherwise Assimp may attempt to find minizip on the filesystem using
+    # pkgconfig, but in a poor way that expects just yelling -lminizip at the
+    # linker without any link directories would work. It won't. (The variable
+    # is not an option() so no need to CACHE it.)
+    set(ASSIMP_BUILD_MINIZIP ON)
     add_subdirectory("${DEPS_DIR}/assimp")
 
     # Help FindAssimp locate everything

Funnily enough it doesn't seem to actually use it for anything (or at least that's what GitHub search tells me). I don't have any easy way to reproduce the original problem, can you check if this fixes it and the link_directories() call is no longer needed?

[Skylion007]

It looks like it is only used in Hunter Builds, otherwise it's not needed or used anywhere.

[erikwijmans]

@dhruvbatra can you try with these changes?

[dhruvbatra]

Yes, confirming that this works. Thanks @erikwijmans and @mosra.