Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve toggling between static/dynamic linking of dependency libraries #2089

Open
taketwo opened this issue Nov 15, 2017 · 14 comments
Open
Assignees
Labels
kind: todo Type of issue module: cmake skill: cmake Skills/areas of expertise needed to tackle the issue

Comments

@taketwo
Copy link
Member

taketwo commented Nov 15, 2017

Follow-up to discussions in #2084.

Plan:

  1. Remove options
  • PCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32
  • PCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32
  1. Introduce options
    • BOOST_USE_STATIC
    • FLANN_USE_STATIC
    • QHULL_USE_STATIC

Open questions:

  1. Do these options express strict requirement or just a preference? For example, if the user set FLANN_USE_STATIC=TRUE, but only dynamic libraries are available, is this a configuration error? Or do we issue a warning and coerce the option to FALSE?
  2. How do these options interact with PCL_SHARED_LIBS? If shared libs are disabled, i.e. the user wants to compile PCL as a static library, should it force all the dependencies to be static as well?
  3. Default values? Depending on the platform?

The second question may involve some complicated logic. Stepping a bit back, I wonder if such level of configurability is actually needed? Maybe PCL_SHARED_LIBS is sufficient? If it is set, then we default to finding shared dependencies and fall back to static. If it is not set, then we default to static and fall back to shared (or even error)?

@taketwo taketwo self-assigned this Nov 15, 2017
@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Nov 16, 2017

Answering the first block of questions:

  1. Strict requirements.
  2. Are you not automatically required to do that? If I recall correctly I don't think you're able to link a static lib against a shared lib. Something to take into account.
    https://stackoverflow.com/questions/8547277/link-dll-to-static-library-and-load-it-into-an-application-linked-against-the-sa
  3. Shared libs. I think that if you want static linkage you really need to know what you're doing and that's why I would not set is as default.

Second questions:

If it is set, then we default to finding shared dependencies and fall back to static.

Agreed

If it is not set, then we default to static and fall back to shared (or even error)

Default to static and error if not found. I'm still convinced you can't link a static against a dynamic.

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Nov 16, 2017

I stand corrected
https://gist.github.com/SergioRAgostinho/7463124494ffb480ab0c3ac1963105af
This just dumped

$ makeScanning dependencies of target dynamic
[ 16%] Building CXX object CMakeFiles/dynamic.dir/dynamic.cpp.o
[ 33%] Linking CXX shared library libdynamic.so
[ 33%] Built target dynamic
Scanning dependencies of target static
[ 50%] Building CXX object CMakeFiles/static.dir/static.cpp.o
[ 66%] Linking CXX static library libstatic.a
[ 66%] Built target static
Scanning dependencies of target executable
[ 83%] Building CXX object CMakeFiles/executable.dir/executable.cpp.o
[100%] Linking CXX executable executable
[100%] Built target executable
$ ./executable 
1
-1
$ ldd executable 
	linux-vdso.so.1 =>  (0x00007ffd6293e000)
	libdynamic.so => <path>/link_static_against_dynamic/build/libdynamic.so (0x00007f0002ea5000)
	libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f0002ae4000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f000271a000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f0002411000)
	/lib64/ld-linux-x86-64.so.2 (0x000055e796ba6000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f00021fb000)

I don't understand why I had problem with this before then...

@taketwo
Copy link
Member Author

taketwo commented Nov 16, 2017

Hm, I'm afraid we don't have complete understanding of the use cases and the reasons why these options were introduced. For example, with Boost the explicit request for dynamic linking PCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32=ON on Windows overrides PCL_SHARED_LIB=OFF.

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Nov 16, 2017

I would say we ask @UnaNancyOwen to give it a try to some of these Windows cases individually and report what's the outcome, starting with that one you just mentioned.

@UnaNancyOwen
Copy link
Member

What should I try and report?

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Nov 16, 2017

You'll need to modify the cmake file, but the idea is to set PCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32=ON and build pcl as a static library. Just to be sure, try to compile and run some of the unit tests.

@UnaNancyOwen
Copy link
Member

UnaNancyOwen commented Nov 17, 2017

PCL can build normally with these options on Windows.
And, I confirmed that test_common can build and run successfully.

//Build against a dynamically linked Boost on Win32 platforms.
PCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32:BOOL=ON

//Build shared libraries.
PCL_SHARED_LIBS:BOOL=OFF
//Boost atomic library (debug)
Boost_ATOMIC_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_atomic-vc141-mt-gd-1_64.lib

//Boost atomic library (release)
Boost_ATOMIC_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_atomic-vc141-mt-1_64.lib

//Boost chrono library (debug)
Boost_CHRONO_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_chrono-vc141-mt-gd-1_64.lib

//Boost chrono library (release)
Boost_CHRONO_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_chrono-vc141-mt-1_64.lib

//Boost date_time library (debug)
Boost_DATE_TIME_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_date_time-vc141-mt-gd-1_64.lib

//Boost date_time library (release)
Boost_DATE_TIME_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_date_time-vc141-mt-1_64.lib

//Boost filesystem library (debug)
Boost_FILESYSTEM_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_filesystem-vc141-mt-gd-1_64.lib

//Boost filesystem library (release)
Boost_FILESYSTEM_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_filesystem-vc141-mt-1_64.lib

//Path to a file.
Boost_INCLUDE_DIR:PATH=C:/Program Files/Boost/include/boost-1_64

//Boost iostreams library (debug)
Boost_IOSTREAMS_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_iostreams-vc141-mt-gd-1_64.lib

//Boost iostreams library (release)
Boost_IOSTREAMS_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_iostreams-vc141-mt-1_64.lib

//Boost library directory DEBUG
Boost_LIBRARY_DIR_DEBUG:PATH=C:/Program Files/Boost/lib

//Boost library directory RELEASE
Boost_LIBRARY_DIR_RELEASE:PATH=C:/Program Files/Boost/lib

//Boost mpi library (debug)
Boost_MPI_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_mpi-vc141-mt-gd-1_64.lib

//Boost mpi library (release)
Boost_MPI_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_mpi-vc141-mt-1_64.lib

//Boost regex library (debug)
Boost_REGEX_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_regex-vc141-mt-gd-1_64.lib

//Boost regex library (release)
Boost_REGEX_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_regex-vc141-mt-1_64.lib

//Boost serialization library (debug)
Boost_SERIALIZATION_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_serialization-vc141-mt-gd-1_64.lib

//Boost serialization library (release)
Boost_SERIALIZATION_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_serialization-vc141-mt-1_64.lib

//Boost system library (debug)
Boost_SYSTEM_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_system-vc141-mt-gd-1_64.lib

//Boost system library (release)
Boost_SYSTEM_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_system-vc141-mt-1_64.lib

//Boost thread library (debug)
Boost_THREAD_LIBRARY_DEBUG:FILEPATH=C:/Program Files/Boost/lib/boost_thread-vc141-mt-gd-1_64.lib

//Boost thread library (release)
Boost_THREAD_LIBRARY_RELEASE:FILEPATH=C:/Program Files/Boost/lib/boost_thread-vc141-mt-1_64.lib

@SergioRAgostinho
Copy link
Member

Was PCL actually build as a static lib as well?

@UnaNancyOwen
Copy link
Member

Yes, PCL was built as a static library.

@taketwo
Copy link
Member Author

taketwo commented Nov 18, 2017

Thanks for trying this out, Tsukasa.

So it seems like all possible combinations of static/dynamic settings are valid on all platforms. Therefore, I propose the following solution, which on one hand is zero-configuration for those who don't care, and utmost flexible for those who care.

  1. Add PCL_BUILD_WITH_xxx_LINKING options that can assume three values: STATIC, DYNAMIC, and AUTO.
  2. If such an option is set to AUTO (which will be the default), then the decision which type to link against will depend on the value of PCL_SHARED_LIBS. However, if no matching type is installed on the system, then will fall-back silently to the other type.
  3. If such an option is set to DYNAMIC, but dynamically linked version is not installed on the system, this is an error. No fall-back happens. Same for static.

Suggestions for a better name of the new options are welcome.

@SergioRAgostinho
Copy link
Member

Agreed. It's a very solid approach.

@UnaNancyOwen
Copy link
Member

Looks good proposal.

@stale
Copy link

stale bot commented May 19, 2020

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the status: stale label May 19, 2020
@kunaltyagi kunaltyagi added kind: todo Type of issue skill: cmake Skills/areas of expertise needed to tackle the issue labels May 24, 2020
@stale stale bot removed the status: stale label May 24, 2020
@kunaltyagi
Copy link
Member

@shrijitsingh99 please read this discussion since you're fiddling with boost related options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: todo Type of issue module: cmake skill: cmake Skills/areas of expertise needed to tackle the issue
Projects
None yet
Development

No branches or pull requests

4 participants