noloader / cryptopp-cmake

CMake files for Crypto++ project
BSD 3-Clause "New" or "Revised" License
92 stars 68 forks source link

Duplicate and wrongly added compiler definitions #90

Closed abdes closed 2 years ago

abdes commented 2 years ago

Compiler definitions (e.g. -DCRYPTOPP_DATA_DIR=...) and options (e.g. /DWIN32 and /D_WINDOWS) are being added here:

#============================================================================
# Compiler flags
#============================================================================

# add_compile_definitions added in CMake 3.12
if (${CMAKE_VERSION} VERSION_LESS "3.12")
  # https://stackoverflow.com/q/61250087
  add_definitions(${CMAKE_CPP_FLAGS} ${CRYPTOPP_COMPILE_DEFINITIONS} ${CRYPTOPP_COMPILE_OPTIONS})
else()
  # Fucking CMake blows. It does not honor add_compile_definitions.
  add_compile_definitions(${CMAKE_CPP_FLAGS} ${CRYPTOPP_COMPILE_DEFINITIONS})
  add_compile_options(${CMAKE_CXX_FLAGS} ${CRYPTOPP_COMPILE_DEFINITIONS} ${CRYPTOPP_COMPILE_OPTIONS})
endif()

and here

function(cryptopp_target_compile_properties target)
  # CMake >= 2.8.12
  if (NOT ${CMAKE_VERSION} VERSION_LESS "2.8.12")
    SET(CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} ${CRYPTOPP_COMPILE_OPTIONS}")
  else()
    string (REPLACE ";" " " PROP_STR "${CRYPTOPP_COMPILE_OPTIONS}")
    set_target_properties(${target} PROPERTIES COMPILE_FLAGS "${PROP_STR}")
  endif()
  # CMake >= 2.8.11
  if (NOT ${CMAKE_VERSION} VERSION_LESS "2.8.11")
    target_compile_definitions(${target} PUBLIC ${CRYPTOPP_COMPILE_DEFINITIONS})
  else()
    string (REPLACE ";" " " PROP_STR "${CRYPTOPP_COMPILE_DEFINITIONS}")
    set_target_properties(${target} PROPERTIES COMPILE_DEFINITIONS "${CRYPTOPP_COMPILE_DEFINITIONS}")
  endif()
endfunction()

The first is a really bad way to globally add and duplicate EVERYTHING for EVERYTHING and is necessary as the proper way is being done below by calling the cryptopp_target_compile_properties function for static, shared, etc targets.

A few issues are actually inside the function `` itself:

  1. COMPILE_FLAGS: This property is deprecated. Use the COMPILE_OPTIONS property instead.
  2. Both COMPILE_OPTIONS and COMPILE_DEFINITIONS properties take a semicolon separated list (cmake list), no need to replace the semicolon with spaces.
  3. for setting the COMPILE_DEFINITIONS option, the PROP_STR is not even being used :-)

Suggestion for fix: Therefore, the suggestion is to remove the section of code adding the options and definitions globally, and cleanup the function that does it per target as following:

function(cryptopp_target_compile_properties target)
  # CMake >= 2.8.12
  if (NOT ${CMAKE_VERSION} VERSION_LESS "2.8.12")
    target_compile_options(${target} PUBLIC ${CRYPTOPP_COMPILE_OPTIONS})
  else()
    set_target_properties(${target} PROPERTIES COMPILE_OPTIONS ${CRYPTOPP_COMPILE_OPTIONS})
  endif()
  # CMake >= 2.8.11
  if (NOT ${CMAKE_VERSION} VERSION_LESS "2.8.11")
    target_compile_definitions(${target} PUBLIC ${CRYPTOPP_COMPILE_DEFINITIONS})
  else()
    set_target_properties(${target} PROPERTIES COMPILE_DEFINITIONS ${CRYPTOPP_COMPILE_DEFINITIONS})
  endif()
endfunction()