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

0001-CMake-Build-Fixes.patch #14

Open
orbisvicis opened this issue Jan 22, 2014 · 8 comments
Open

0001-CMake-Build-Fixes.patch #14

orbisvicis opened this issue Jan 22, 2014 · 8 comments

Comments

@orbisvicis
Copy link

From 1e9e03b9eab1c21c8c720c5482feaac3d65984e8 Mon Sep 17 00:00:00 2001
From: Yclept Nemo <-------------------->
Date: Wed, 22 Jan 2014 14:18:36 -0500
Subject: [PATCH] CMake Build Fixes

    * Combine shared/static configuration. Only one instance of
    libflandmark is now built; Use -DBUILD_SHARED_LIBS:BOOL to toggle
    which one. Examples will link against whichever library is built.
    * Enable position-idependent-code in an agnostic fashion; PIC is now
    default for both static (required for linking against shared
    libraries) and shared (required) builds on all supported CMake
    compilers.
    * Add SOVERSION and VERSION information (based on
    flandmark_VERSION_{MAJOR,MINOR}). Symlinks are now created when
    building and the soname is embedded in the object file.
    ***** flandmark_VERSION_MAJOR is now tied to the SOVERSION *****
    * Remove unused COMPILE_DEFINITIONS property: FLANDMARK_STATIC

---
 examples/CMakeLists.txt     |  6 +++---
 libflandmark/CMakeLists.txt | 19 +++++++++----------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt
index 7e93091..89f549c 100644
--- a/examples/CMakeLists.txt
+++ b/examples/CMakeLists.txt
@@ -27,13 +27,13 @@ endif(${OpenCV_VERSION_MINOR} LESS 3)

 set(${PROJECT_NAME}_simple_example_srcs simple_example.cpp)
 add_executable(${PROJECT_NAME}_simple_example ${${PROJECT_NAME}_simple_example_srcs})
-target_link_libraries(${PROJECT_NAME}_simple_example flandmark_static ${CV_LIBS_1})
+target_link_libraries(${PROJECT_NAME}_simple_example flandmark ${CV_LIBS_1})

 set(${PROJECT_NAME}_example1_srcs example1.cpp)
 add_executable(${PROJECT_NAME}_1 ${${PROJECT_NAME}_example1_srcs})
-target_link_libraries(${PROJECT_NAME}_1 flandmark_static ${CV_LIBS_1} ${CV_LIBS_2})
+target_link_libraries(${PROJECT_NAME}_1 flandmark ${CV_LIBS_1} ${CV_LIBS_2})

 set(${PROJECT_NAME}_example2_srcs example2.cpp)
 add_executable(${PROJECT_NAME}_2 ${${PROJECT_NAME}_example2_srcs})
-target_link_libraries(${PROJECT_NAME}_2 flandmark_static ${CV_LIBS_1} ${CV_LIBS_2})
+target_link_libraries(${PROJECT_NAME}_2 flandmark ${CV_LIBS_1} ${CV_LIBS_2})

diff --git a/libflandmark/CMakeLists.txt b/libflandmark/CMakeLists.txt
index 8acfefc..e707f26 100644
--- a/libflandmark/CMakeLists.txt
+++ b/libflandmark/CMakeLists.txt
@@ -1,20 +1,19 @@
 find_package( OpenCV REQUIRED )
 include_directories(${OpenCV_INCLUDE_DIRS})

-add_library(flandmark_static STATIC flandmark_detector.cpp flandmark_detector.h liblbp.cpp liblbp.h)
-target_link_libraries(flandmark_static ${OpenCV_LIBS})
-if(CMAKE_COMPILER_IS_GNUCC)
-    set_target_properties(flandmark_static PROPERTIES COMPILE_FLAGS -fPIC)
-endif(CMAKE_COMPILER_IS_GNUCC)
-set_property(TARGET flandmark_static PROPERTY COMPILE_DEFINITIONS FLANDMARK_STATIC)
-
-add_library(flandmark_shared SHARED flandmark_detector.cpp flandmark_detector.h liblbp.cpp liblbp.h)
-target_link_libraries(flandmark_shared ${OpenCV_LIBS})
+add_library(flandmark flandmark_detector.cpp flandmark_detector.h liblbp.cpp liblbp.h)
+target_link_libraries(flandmark ${OpenCV_LIBS})
+set_target_properties(flandmark PROPERTIES POSITION_INDEPENDENT_CODE TRUE)
+set_target_properties(flandmark PROPERTIES
+  SOVERSION "${flandmark_VERSION_MAJOR}"
+  VERSION "${flandmark_VERSION_MAJOR}.${flandmark_VERSION_MINOR}"
+)

 #setup Config.cmake
 SET(FLANDMARK_BASE_DIR "${PROJECT_SOURCE_DIR}/libflandmark")
 set(FLANDMARK_BINARY_DIR "${PROJECT_BINARY_DIR}/libflandmark")
 configure_file(flandmarkConfig.cmake.in
-  "${PROJECT_BINARY_DIR}/libflandmark/flandmarkConfig.cmake" @ONLY)
+  "${PROJECT_BINARY_DIR}/libflandmark/flandmarkConfig.cmake" @ONLY
+)

 export(PACKAGE flandmark)
-- 
1.8.2.2

@orbisvicis
Copy link
Author

@sthalik: this will affect headtracker's build.

@sthalik
Copy link
Contributor

sthalik commented Jan 22, 2014

In the diff posted, can't find a cache variable declaration, that would control static/shared build type.

Is shared object version information really necessary, given how the api's relatively stable?

Thanks for the notice about headtracker breakage, in any case.

Overall, don't see how the patch improves on things, with the exception of PIC enablement in a generic fashion, but final say of course goes to @uricamic !

@orbisvicis
Copy link
Author

I'm not sure what you mean, BUILD_SHARED_LIBS is a builtin CMake variable that defaults to FALSE (build static libs). It is a cache variable in the sense that it is saved in CMakeCache.txt

Irrespective of the api, which I haven't really examined, soversion is also useful to specify version compatibility between forks of flandmark.

I like the changes because they simplify CMakeLists.txt as well as building on linux (I've uploaded an Arch Linux package @ https://aur.archlinux.org/packages/flandmark-git/):
* control which version the examples link against
* name-scheme (shared name) common (not ubiqitous) on linux
* name-scheme incompatible on windows (static/shared have same extension) but not a problem because only one version built

But of course final say and etc... !

@uricamic
Copy link
Owner

@orbisvicis: thanks for the patch, I tried it on the new universal version of flandmark (not publicly available yet) and I really like how it simplified several things.
However, there are some things:

  • set_target_properties(flandmark PROPERTIES POSITION_INDEPENDENT_CODE TRUE) <- this requires CMake at version 2.8.9 at least, before that version the property POSITION_INDEPENDENT_CODE did not exist.
  • I don't know why BUILD_SHARED_LIBS option is not listed even among advanced options in ccmake, so maybe it will be better to add this option and leave the user to decide whether she/he wants to have static or dynamic build. I guess this is what @sthalik meant?

I am now busy with the new project, but I will probably incorporate this patch here soon.

Anyways, thanks for the patch, if not here, it will be definitely useful in the upcoming projec.

@orbisvicis
Copy link
Author

The following patch addresses the second point, as for the first I'm not really sure if you want to bump the minimum required CMake version or support both approaches or only the former:

From 9432735689a47f97da98924b0705816a5e147f4e Mon Sep 17 00:00:00 2001
From: Yclept Nemo <-------------------->
Date: Thu, 23 Jan 2014 10:39:16 -0500
Subject: [PATCH] CMake: change variables shown in configuration GUI

    hide: CMAKE_INSTALL_PREFIX
    show: BUILD_SHARED_LIBS (default TRUE), CMAKE_SKIP_BUILD_RPATH
          (default FALSE)
---
 CMakeLists.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0e55a45..4637deb 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -6,6 +6,11 @@ project(flandmark)
 set(flandmark_VERSION_MAJOR 1)
 set(flandmark_VERSION_MINOR 7)

+# configure which options are shown in the gui
+MARK_AS_ADVANCED(CMAKE_INSTALL_PREFIX)
+SET(BUILD_SHARED_LIBS "TRUE" CACHE BOOL "Global flag to cause add_library to create shared libraries if on.")
+SET(CMAKE_SKIP_BUILD_RPATH "FALSE" CACHE BOOL "Do not include RPATHs in the build tree.")
+
 if(NOT CMAKE_BUILD_TYPE)
    set(CMAKE_BUILD_TYPE RELEASE CACHE STRING
        "Choose the type of build, options are: None Debug Release RelWithDebInfo MinSizeRel."
-- 
1.8.2.2

@orbisvicis
Copy link
Author

More changes:

From 300c02a97728d3c74711cd001da1d294288e6be6 Mon Sep 17 00:00:00 2001
From: Yclept Nemo <-------------------->
Date: Sat, 25 Jan 2014 10:28:06 -0500
Subject: [PATCH 1/2] Avoid conflict between shared/static libs in MSVC

    Prepend the "lib" prefix when building static libraries. This only
    affects MSVC, all other platforms already default to the "lib"
    prefix.
    By default import libraries and static libraries share the same name
    in MSVC. This patch avoids name conflicts between shared and static
    libraries built in the same tree using MSVC:
        shared: flandmark.dll
        import: flandmark.lib
        static: libflandmark.lib
---
 libflandmark/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libflandmark/CMakeLists.txt b/libflandmark/CMakeLists.txt
index e707f26..ddfa0e1 100644
--- a/libflandmark/CMakeLists.txt
+++ b/libflandmark/CMakeLists.txt
@@ -2,6 +2,9 @@ find_package( OpenCV REQUIRED )
 include_directories(${OpenCV_INCLUDE_DIRS})

 add_library(flandmark flandmark_detector.cpp flandmark_detector.h liblbp.cpp liblbp.h)
+if(NOT ${BUILD_SHARED_LIBS})
+  set_target_properties(flandmark PROPERTIES PREFIX "lib")
+endif()
 target_link_libraries(flandmark ${OpenCV_LIBS})
 set_target_properties(flandmark PROPERTIES POSITION_INDEPENDENT_CODE TRUE)
 set_target_properties(flandmark PROPERTIES
-- 
1.8.2.2

Now that there are no naming conflicts, the following expects CMake output target names to be honored in order to work. @sthalik: you should use FindFLANDMARK.cmake in headtracker's CMakeLists.cmake if you want to be able to specify static/shared versions of flandmark, rather than flandmarkConfig.cmake which will only reference the last version built (ie assume flandmarkConfig.cmake contains the packager's prefered version to link against).

From 2a0ea0178025232eb544e942be108196e792cae0 Mon Sep 17 00:00:00 2001
From: Yclept Nemo <-------------------->
Date: Sat, 25 Jan 2014 12:29:45 -0500
Subject: [PATCH 2/2] Add a CMake find module for flandmark

---
 libflandmark/FindFLANDMARK.cmake | 60 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 libflandmark/FindFLANDMARK.cmake

diff --git a/libflandmark/FindFLANDMARK.cmake b/libflandmark/FindFLANDMARK.cmake
new file mode 100644
index 0000000..507fa3c
--- /dev/null
+++ b/libflandmark/FindFLANDMARK.cmake
@@ -0,0 +1,60 @@
+# - Try to find the flandmark facial landmark detector library
+#
+# =============================================================================
+# Once done this will define:
+#
+#  FLANDMARK_FOUND          TRUE if found; FALSE otherwise
+#  FLANDMARK_INCLUDE_DIRS   where to find flandmark_detector.h
+#  FLANDMARK_LIBRARIES      the libraries to link against
+#
+# =============================================================================
+# Variables used by this module:
+#
+#  FLANDMARK_PREFER_STATIC  If TRUE and available, link against the static
+#                           flandmark library. Otherwise select the shared
+#                           version
+#
+# =============================================================================
+# To use this from another project:
+#
+# create a directory named cmake/Modules under the project root, copy this file
+# (FindFLANDMARK.cmake) there, and in the top-level CMakeLists.txt include:
+#   set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH}
+#                         "${CMAKE_SOURCE_DIR}/cmake/Modules/")
+#
+# =============================================================================
+
+find_path(FLANDMARK_INCLUDE_DIR flandmark_detector.h)
+
+set(CMAKE_FIND_LIBRARY_SUFFIXES ".so" ".dll")
+find_library(FLANDMARK_LIBRARY_STATIC NAMES flandmark)
+
+set(CMAKE_FIND_LIBRARY_PREFIXES "lib")
+set(CMAKE_FIND_LIBRARY_SUFFIXES ".a" ".lib")
+find_library(FLANDMARK_LIBRARY_SHARED NAMES flandmark)
+
+if(FLANDMARK_LIBRARY_STATIC and FLANDMARK_LIBRARY_SHARED)
+  if(FLANDMARK_PREFER_STATIC)
+    set(FLANDMARK_LIBRARY ${FLANDMARK_LIBRARY_STATIC})
+  else()
+    set(FLANDMARK_LIBRARY ${FLANDMARK_LIBRARY_SHARED})
+  endif()
+elseif(FLANDMARK_LIBRARY_STATIC)
+  set(FLANDMARK_LIBRARY ${FLANDMARK_LIBRARY_STATIC})
+elseif(FLANDMARK_LIBRARY_SHARED)
+  set(FLANDMARK_LIBRARY ${FLANDMARK_LIBRARY_SHARED})
+else()
+  set(FLANDMARK_LIBRARY "FLANDMARK_LIBRARY-NOTFOUND")
+endif()
+
+set(FLANDMARK_LIBRARIES ${FLANDMARK_LIBRARY})
+set(FLANDMARK_INCLUDE_DIRS ${FLANDMARK_INCLUDE_DIR})
+
+include(FindPackageHandleStandardArgs)
+# handle the QUIETLY and REQUIRED arguments and set FLANDMARK_FOUND to TRUE
+# if all listed variables are TRUE
+find_package_handle_standard_args(FLANDMARK
+                                  DEFAULT_MSG
+                                  FLANDMARK_LIBRARY FLANDMARK_INCLUDE_DIR)
+
+mark_as_advanced(FLANDMARK_INCLUDE_DIR FLANDMARK_LIBRARY)
-- 
1.8.2.2

In addition to simplifying flandmarkConfig.cmake, provide absolute paths like find_library():

From 39976612539a8e82b7179aeee661c94369578e13 Mon Sep 17 00:00:00 2001
From: Yclept Nemo <-------------------->
Date: Sat, 25 Jan 2014 14:18:23 -0500
Subject: [PATCH 1/2] Simplify flandmarkConfig.cmake.in

---
 libflandmark/CMakeLists.txt           |  4 ++--
 libflandmark/flandmarkConfig.cmake.in | 12 +++---------
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/libflandmark/CMakeLists.txt b/libflandmark/CMakeLists.txt
index ddfa0e1..671dc6f 100644
--- a/libflandmark/CMakeLists.txt
+++ b/libflandmark/CMakeLists.txt
@@ -13,8 +13,8 @@ set_target_properties(flandmark PROPERTIES
 )

 #setup Config.cmake
-SET(FLANDMARK_BASE_DIR "${PROJECT_SOURCE_DIR}/libflandmark")
-set(FLANDMARK_BINARY_DIR "${PROJECT_BINARY_DIR}/libflandmark")
+SET(FLANDMARK_INCLUDE_DIRS "${PROJECT_SOURCE_DIR}/libflandmark")
+get_property(FLANDMARK_LIBRARIES TARGET flandmark PROPERTY LOCATION)
 configure_file(flandmarkConfig.cmake.in
   "${PROJECT_BINARY_DIR}/libflandmark/flandmarkConfig.cmake" @ONLY
 )
diff --git a/libflandmark/flandmarkConfig.cmake.in b/libflandmark/flandmarkConfig.cmake.in
index e0fad9b..cd4f95e 100644
--- a/libflandmark/flandmarkConfig.cmake.in
+++ b/libflandmark/flandmarkConfig.cmake.in
@@ -1,11 +1,5 @@
-SET(FLANDMARK_BASE_DIR "@FLANDMARK_BASE_DIR@")
-SET(FLANDMARK_BINARY_DIR "@FLANDMARK_BINARY_DIR@")
+SET(FLANDMARK_INCLUDE_DIRS "@FLANDMARK_INCLUDE_DIRS@")

-SET(FLANDMARK_INCLUDE_DIRS ${FLANDMARK_BASE_DIR})
-
-SET(FLANDMARK_LINK_DIRS ${FLANDMARK_BINARY_DIR} ${FLANDMARK_BINARY_DIR}/Release)
-
-SET(FLANDMARK_LIBRARIES flandmark_shared)
-
-SET(FLANDMARK_FOUND 1)
+SET(FLANDMARK_LIBRARIES "@FLANDMARK_LIBRARIES@")

+SET(FLANDMARK_FOUND "TRUE")
-- 
1.8.2.2

...

From 2ab7cd8d656022152b01f73b7a0fa4fc8fc5d3bd Mon Sep 17 00:00:00 2001
From: Yclept Nemo <-------------------->
Date: Sat, 25 Jan 2014 14:24:36 -0500
Subject: [PATCH 2/2] Update example external CMakeLists.txt

    track previous commit 39976612539a8e82b7179aeee661c94369578e13
---
 examples/external/CMakeLists.txt | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/examples/external/CMakeLists.txt b/examples/external/CMakeLists.txt
index 36ae912..7e84bc5 100644
--- a/examples/external/CMakeLists.txt
+++ b/examples/external/CMakeLists.txt
@@ -41,7 +41,7 @@ SET(CMAKE_VERBOSE_MAKEFILE ON)
 SET(CMAKE_BUILD_TYPE "Debug")
 SET(CMAKE_CXX_FLAGS "-D_ENGLISH")

-######## INCLUDE AND LINK DIRECTORIES ########
+######## INCLUDE DIRECTORIES ########

 INCLUDE_DIRECTORIES(
    /usr/local/include
@@ -51,11 +51,6 @@ INCLUDE_DIRECTORIES(
   ${FLANDMARK_INCLUDE_DIRS}
 )

-LINK_DIRECTORIES(
-   ${FLANDMARK_LINK_DIRS}
-   ${OpenCV_LIB_DIR}
-)
-

 ######## MAIN EXECUTABLES ########

-- 
1.8.2.2

Ok, that's that. I don't expect to post any more patches. FTR, I haven't tested any of these on windows.

@orbisvicis
Copy link
Author

Oops.

From e286e8e3d7b9e3d2d3b443bbf1ef156d8c84c985 Mon Sep 17 00:00:00 2001
From: Yclept Nemo <-------------------->
Date: Sat, 25 Jan 2014 15:43:36 -0500
Subject: [PATCH] Require CMake >= 2.8 to avoid CMP0012 warnings

---
 CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4637deb..789e537 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 2.6)
+cmake_minimum_required(VERSION 2.8)

 project(flandmark)

-- 
1.8.2.2

@sthalik
Copy link
Contributor

sthalik commented Jun 5, 2014

Could we at the very least not rename the flandmark_static library? This violates POLA.

@uricamic what is status on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants