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

CMake improvements #94

Open
chafey opened this issue Dec 20, 2022 · 6 comments
Open

CMake improvements #94

chafey opened this issue Dec 20, 2022 · 6 comments

Comments

@chafey
Copy link
Contributor

chafey commented Dec 20, 2022

  • should update to follow "Modern CMake" (google search for this, lots of resources)
    • remove use of GLOB
    • create CMakeLists.txt for subdirectories (core lib, apps, tests)
    • create separate CMakeLIsts.txt for WASM build
    • eliminate use of project globals
      • setting CMAKE_CXX_FLAGS (set target properties or use cmake presets)
      • include_directories
@kmilos
Copy link

kmilos commented Jan 4, 2024

In addition, the current list is not very portable - notably it seems it was only tested w/ MSVC on Windows but not w/ MinGW. I had to make the following changes to 0.9.0 for example:

--- CMakeLists.txt.orig	2022-05-25 14:38:19.000000000 +0200
+++ CMakeLists.txt	2024-01-04 13:57:38.007707400 +0100
@@ -1,11 +1,5 @@
 cmake_minimum_required(VERSION 3.10.0)
 
-project (openjph DESCRIPTION "Open source implementation of JPH" LANGUAGES CXX)
-
-################################################################################################
-# Building OpenJPH
-################################################################################################
-
 ############################################################
 # Parse version file
 # credit: https://stackoverflow.com/a/47084079
@@ -25,6 +19,12 @@
 set(OPENJPH_VERSION "${OPENJPH_VERSION_MAJOR}.${OPENJPH_VERSION_MINOR}.${OPENJPH_VERSION_PATCH}")
 ############################################################
 
+project (openjph VERSION ${OPENJPH_VERSION} DESCRIPTION "Open source implementation of JPH" LANGUAGES CXX)
+
+################################################################################################
+# Building OpenJPH
+################################################################################################
+
 option(OJPH_DISABLE_INTEL_SIMD "Disables the use of SIMD instructions and associated files" OFF)
 option(BUILD_SHARED_LIBS "Shared Libraries" ON)
 option(OJPH_ENABLE_TIFF_SUPPORT "Enables input and output support for TIFF files" ON)
@@ -51,17 +51,10 @@
 	endif()
 endif()
 
-if (BUILD_SHARED_LIBS AND MSVC)
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /D \"OJPH_BUILD_SHARED_LIBRARY\"")
-endif()
-
 if (OJPH_CODE_COVERAGE AND NOT MSVC)
 	set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --coverage")
 endif()
 
-set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/../bin)
-set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/../bin)
-
 include_directories(src/core/common)
 include_directories(src/apps/common)
 
@@ -94,9 +87,13 @@
 source_group("others"            FILES ${OTHERS})
 source_group("transform"         FILES ${TRANSFORM})
 
+set(PKG_CONFIG_INCLUDEDIR "\${prefix}/include")
+set(PKG_CONFIG_LIBDIR "\${prefix}/lib")
+set(PKG_CONFIG_LIBS "-lopenjph")
+
 configure_file(
   "${CMAKE_CURRENT_SOURCE_DIR}/src/pkg-config.pc.cmake"
-  "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/${PROJECT_NAME}.pc"
+  "${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pc"
 )
 
 if(EMSCRIPTEN)
@@ -125,8 +122,12 @@
 
 target_compile_definitions(openjph PUBLIC _FILE_OFFSET_BITS=64)
 
+if (BUILD_SHARED_LIBS AND WIN32)
+  target_compile_definitions(openjph PRIVATE OJPH_BUILD_SHARED_LIBRARY)
+endif()
+
 if (OPENJPH_VERSION)
-  if (WIN32)
+  if (MSVC)
     set_target_properties(openjph
       PROPERTIES
         OUTPUT_NAME "openjph.${OPENJPH_VERSION_MAJOR}.${OPENJPH_VERSION_MINOR}")
@@ -160,7 +161,7 @@
 ############################################################
 if( OJPH_ENABLE_TIFF_SUPPORT )
 
-  if( WIN32 )
+  if( MSVC )
 
     set(TIFF_INCLUDE_DIR "C:\\Program Files\\tiff\\include" CACHE PATH "the directory containing the TIFF headers")
     set(TIFF_LIBRARY_DEBUG   "C:\\Program Files\\tiff\\lib\\tiffd.lib" CACHE FILEPATH "the path to the TIFF library for debug configurations")
@@ -175,7 +176,7 @@
     message( STATUS "   TIFFXX_LIBRARY_DEBUG : \"${TIFFXX_LIBRARY_DEBUG}\"  " )
     message( STATUS "   TIFFXX_LIBRARY_RELEASE : \"${TIFFXX_LIBRARY_RELEASE}\"  " )
 
-  endif( WIN32 )
+  endif( MSVC )
 
   FIND_PACKAGE( TIFF )
 
@@ -235,15 +236,17 @@
 	DESTINATION bin)
 
 include(GNUInstallDirs)
-install(TARGETS openjph LIBRARY
-  DESTINATION ${CMAKE_INSTALL_LIBDIR})
+install(TARGETS openjph
+  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
+  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR})
 
 install (DIRECTORY src/core/common/
   DESTINATION include/openjph
   FILES_MATCHING
   PATTERN "*.h")
 
-install(FILES "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/${PROJECT_NAME}.pc"
+install(FILES "${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pc"
   DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)
 
 ################################################################################################

@aous72
Copy link
Owner

aous72 commented Jan 4, 2024

Hi Milos,

Thank you for your suggestions. I will look into them tonight or tomorrow.

I am in the process of modifying CMake files to have better arrangement. You can find the modified files in the development branch. If you have any thoughts let me know.

I will try to integrate your suggestions, and if I there are any questions, I will come back to you.

Kind regards,
Aous.

@kmilos
Copy link

kmilos commented Jan 8, 2024

0.10.x was a step in the right direction, thanks.

I'm still carrying a few differences when packaging for MINGW though that you might want to consider in the longer term: https://github.com/msys2/MINGW-packages/blob/0fe1e8ed82ff8f4f84c63de361d1619cc32e3b79/mingw-w64-openjph/0001-cmake-mingw-fixes.patch

@aous72
Copy link
Owner

aous72 commented Jan 9, 2024

Thank you for your guidance.
I tried to implement all changes except for one.

@fralinjh
Copy link

Hi Aous,
The following change to tests/CMakeLists.txt supports both static and shared libraries for Release and Debug configurations on Windows using Visual Studio 2022. Hope this helps and thank you for your contributions.

46,56c45,49
< if (MSVC)
<   if(CMAKE_BUILD_TYPE MATCHES "Debug")
<     add_custom_command(TARGET test_executables POST_BUILD
<       COMMAND ${CMAKE_COMMAND} -E copy "../bin/Debug/gtest.dll" "./Debug/"
<       COMMAND ${CMAKE_COMMAND} -E copy "../bin/Debug/gtest_main.dll" "./Debug/"
<     )
<   elseif(CMAKE_BUILD_TYPE MATCHES "Release")
<     add_custom_command(TARGET test_executables POST_BUILD
<       COMMAND ${CMAKE_COMMAND} -E copy "../bin/Release/gtest.dll" "./Release/"
<       COMMAND ${CMAKE_COMMAND} -E copy "../bin/Release/gtest_main.dll" "./Release/"
<     )
<   endif()
---
> if (MSVC AND BUILD_SHARED_LIBS)
>   add_custom_command(TARGET test_executables POST_BUILD
>     COMMAND ${CMAKE_COMMAND} -E copy "../bin/\$(Configuration)/gtest.dll" "./\$(Configuration)/"
>     COMMAND ${CMAKE_COMMAND} -E copy "../bin/\$(Configuration)/gtest_main.dll" "./\$(Configuration)/"
>   )

@aous72
Copy link
Owner

aous72 commented Jan 22, 2024

@fralinjh
Thank you for this useful suggestion -- it shortened the process.

I changed to this now, and I also did a lot of changes to my CMake files, mainly to avoid setting
CMAKE_LIBRARY_OUTPUT_DIRECTORY
and the like. Now all files are copied after compilation to their desired location.

Now build RUN_TESTS in visual studio performs tests successfully, but tests do not show up in the Test Explorer -- VS requires a different directory structure.

Thank you again.
Aous.

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

4 participants