hungpham2511 / toppra

robotic motion planning library
https://hungpham2511.github.io/toppra/index.html
MIT License
625 stars 170 forks source link

Allow to build static library #178

Closed ahoarau closed 3 years ago

ahoarau commented 3 years ago

This removes the hardcoded SHARED library build, allowing users to choose between static or shared libraries, passing -DBUILD_SHARED_LIBS=True or False.

jmirabel commented 3 years ago

I think this changes the default behavior which isn't desired. The doc says:

BUILD_SHARED_LIBS

Global flag to cause add_library() to create shared libraries if on.

If present and true, this will cause all libraries to be built shared unless the library was explicitly added as a static library. This variable is often added to projects as an option() so that each user of a project can decide if they want to build the project using shared or static libraries.

Can you also add

option(BUILD_SHARED_LIBS 'Toggle between shared and static libs` TRUE)

so that the default is SHARED ?

hungpham2511 commented 3 years ago

Setting up CI on Windows is a great idea. If I am not wrong many of the library users are on Windows.

FYI, I am testing out github action and it has Windows-based runners. But I am not sure how much work is needed to get it to work as I have very minimal experience compiling stuff on Windows.

jmirabel commented 3 years ago

To be clear, the change I request is the default library type. CI on Windows is a different topic that goes into another PR.

It shouldn't be hard to implement either with github action or appveyor. @ahoarau let us know if you are wiling to do this.

ahoarau commented 3 years ago

Basically all we need is those lines to be integrated in the CI script :

git clone https://github.com/hungpham2511/toppra
cd toppra/cpp
cmake -B build-release -DPYTHON_BINDINGS=OFF
cmake --build build-release --target install --config Release -j $(nproc)

I wont be much help regarding the script unfortunately.

ahoarau commented 3 years ago

Any news regarding this ?

jmirabel commented 3 years ago

Putting aside the discussion regarding CI on Windows, you still need to make shared lib the default. Please, add option(BUILD_SHARED_LIBS 'Toggle between shared and static libs TRUE)`

When building static library, it isn't sufficient to do add_library(target STATIC ...). You also have to do set_property(TARGET target PROPERTY POSITION_INDEPENDENT_CODE) (on linux at least). Otherwise you get the kind of error that makes CI crash:

/usr/bin/ld: ../src/libtoppra.a(constraint.cpp.o): relocation R_X86_64_PC32 against symbol `_ZTISt16invalid_argument@@GLIBCXX_3.4' can not be used when making a shared object; recompile with -fPIC

ahoarau commented 3 years ago

done !

ahoarau commented 3 years ago

@jmirabel Sorry didnt read it through. Done !

hungpham2511 commented 3 years ago

Thanks!