la1k / libpredict

A satellite orbit prediction library
GNU General Public License v2.0
40 stars 24 forks source link

building on Mac #110

Closed mahtin closed 2 years ago

mahtin commented 2 years ago

When building on a Mac ...

$ make
...
[ 25%] Linking C shared library libpredict.dylib
ld: unknown option: --version-script=/Users/martin/src/la1k/libpredict/src/libpredict.symver
...
$

Hence the following was needed to build the library on a Mac.

$ git diff
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 0cccd74..16b440a 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -31,8 +31,10 @@ target_link_libraries(predict_static m)

 # Tell the linker to use the symbol version script
 get_property(predict_link_flags TARGET predict PROPERTY LINK_FLAGS)
+if(NOT APPLE)
 set(predict_link_flags "${predict_link_flags} -Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/libpredict.symver")
 set_target_properties(predict PROPERTIES LINK_FLAGS ${predict_link_flags})
+endif()

 install(TARGETS predict predict_static
   LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
$

However, I don't know if that's the correct way to solve this build issue. I'll take advice before PR'ing this. Thanks in advance.

ryeng commented 2 years ago

Hi,

Thanks for the bug report!

I think the correct way is to explicitly check if version scripts are supported (instead of checking which platform it is, with the implicit understanding that some platforms don't support version scripts). I've seen some other projects do that. The question I don't know the answer to is what to do if version scripts aren't supported. It seems wrong to just export all symbols.

mahtin commented 2 years ago

I agree; however, I don't know how to check that ld option from within cmake.

In the meantime, it would be nice to have a working build on Mac's straight from your GitHub release sooner vs later. Symbols aside, I can confirm the code works on a Mac.

Sadly, if you wanted to go down a rabbit-hole, there's also the need to build on BSD in-general. The CMAKE_SYSTEM_NAME option could be used to match FreeBSD; but, that may not be enough. As i said, rabbit-hole.

mahtin commented 2 years ago

I've been pointed to a solution by @job (much appreciated) which removes APPLE and correctly tests for the appropriate ld options via check_linker_flag(). I honestly can't say it's pretty; however, it's well worth a read.

https://reviews.llvm.org/D84559?id=280595

@ryeng - Your call if you want to go this route. I'm happy to test.