novatel / novatel_edie

The EDIE (Encode Decode Interface Engine) software development kit allows interfacing and decoding data output from NovAtel's OEM7 receivers.
MIT License
18 stars 6 forks source link

Restructure library dirs, drop dynamic_* libraries #54

Closed valgur closed 1 month ago

valgur commented 4 months ago

This PR proposes the following changes:

  1. Enable building of shared libraries, controlled via the standard BUILD_SHARED_LIBS CMake variable. For Windows shared library support, I set the WINDOWS_EXPORT_ALL_SYMBOLS property. AFAIK, it can cause issues for heavily templated libraries, but that should not be the case here. This can be made stricter in the future by adding proper export attributes to the library symbols and enabling VISIBILITY_HIDDEN in CMake to match the behavior on non-MSVC compilers.
  2. Drop the dynamic_* libraries. As far as I understand it, these were intended to create Python bindings via the ctypes module, which is why the libraries have a C-style interface. Since there are more viable alternatives for the Python bindings in the form of novatel_ediepy and the Pybind11-based ones in my fork, the usefulness of these libraries is questionable. They provide a much less convenient and safe interface to the library while not being compatible with pure C code either. I experimented a bit with converting them into a proper pure C interface (https://github.com/novatel/novatel_edie/compare/main...valgur:novatel_edie:feature/c_api), but it would lead to a lot of duplication and testing overhead. I don't think converting them to a C API would be well-justified, since the library is probably not compact enough to be used in embedded contexts and it's still very much an option for the users to simply encapsulate the C++ code and create the necessary C wrappers themselves, if necessary. That's how I see it at least, let me know if you disagree or if I'm missing anything.
  3. With the dropping of the dynamic libs, the project structure can be simplified a bit and I would also use it as a chance to give the headers a more conventional structure. After re-organizing, the public headers have the following prefixes:
    • novatel_edie/common/...
    • novatel_edie/decoders/...
    • novatel_edie/stream_interface
    • moved version.h to novatel_edie/version.h
  4. Added a novatel_edie:: prefix to the library targets in CMake and added an aggregate novatel_edie::novatel_edie target. This is mostly in anticipation of planned follow-up PR that sets up the installation and packaging of the project. I updated the project name to use an underscore to match.
  5. Updated include guards to match the new structure. Some of them were way too generic, like COMMON_HPP and LOGGER_HPP.
  6. Moved using nlohmann::json inside the edie:: namespace in the public headers, to keep it namespaced for users who might want to avoid that.
  7. Adjusted the linking of spdlog::spdlog to make sure both spdlog::spdlog and spdlog::spdlog_header_only are not linked by accident.
valgur commented 4 months ago

@jonathanmcdermid @rldoris Pinging, just in case you don't have notifications enabled.

jonathanmcdermid commented 1 month ago

@valgur Hi, sorry it took so long to get around to this review. We've had some different priorities for the last couple months but I am back to working on EDIE. I am nearly ready to approve this MR, but the one issue I have is that since we have some other decoders on our private instance of edie, it would be better to keep the novatel/oem decoder components under their own subfolder, decoders/novatel or decoders/oem instead of just under decoders. Whichever folder name you think is more appropriate can be used. Thanks again for the work you've put into these contributions.

valgur commented 1 month ago

@jonathanmcdermid The PR should be ready for review and merge now. A quick summary of the latest changes:

valgur commented 1 month ago

Decided to add conan create . packaging support as well and refactored the root CMakeLists.txt a bit.

Also added a Findspdlog_setup.cmake module which fetches it from GitHub to support plain system dependencies without Conan or Vcpkg, since the rest are available on most system package managers. I.e. you can now simply do sudo apt-get install nlohmann-json3-dev libspdlog-dev libcpptoml-dev and build with -DUSE_CONAN=FALSE on Ubuntu or Debian.

rldoris commented 1 month ago

Hi @valgur Thank you for your work on this PR! Your contributions have been extremely beneficial to the project.

I’ve tried building the PR locally using both Visual Studio 2022 and the command line with the following versions:

However, I’m encountering the following issue:

Error       CMake Error at cmake/ThirdParty.cmake:26 (message):
  Failed to load CONAN_RUNTIME_LIB_DIRS     C:\Users\r\source\repos\github\novatel_edie\cmake/ThirdParty.cmake  26      

It seems the conan_runtime_paths.cmake file is empty (0 KB).

Here's the content of my conan_toolchain.cmake file:


# Conan automatically generated toolchain file
# DO NOT EDIT MANUALLY, it will be overwritten

# Avoid including toolchain file several times (bad if appending to variables like
#   CMAKE_CXX_FLAGS. See https://github.com/android/ndk/issues/323
include_guard()

message(STATUS "Using Conan toolchain: ${CMAKE_CURRENT_LIST_FILE}")

if(${CMAKE_VERSION} VERSION_LESS "3.15")
    message(FATAL_ERROR "The 'CMakeToolchain' generator only works with CMake >= 3.15")
endif()

########## generic_system block #############
# Definition of system, platform and toolset
#############################################

set(CMAKE_CXX_COMPILER "C:/Program Files/Microsoft Visual Studio/2022/Professional/VC/Tools/MSVC/14.39.33519/bin/Hostx64/x64/cl.exe")

# Definition of VS runtime, defined from build_type, compiler.runtime, compiler.runtime_type
cmake_policy(GET CMP0091 POLICY_CMP0091)
if(NOT "${POLICY_CMP0091}" STREQUAL NEW)
    message(FATAL_ERROR "The CMake policy CMP0091 must be NEW, but is '${POLICY_CMP0091}'")
endif()
set(CMAKE_MSVC_RUNTIME_LIBRARY "$<$<CONFIG:Debug>:MultiThreadedDebugDLL>")

message(STATUS "Conan toolchain: C++ Standard 17 with extensions OFF")
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_EXTENSIONS OFF)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

# Conan conf flags start: 
# Conan conf flags end

foreach(config IN LISTS CMAKE_CONFIGURATION_TYPES)
    string(TOUPPER ${config} config)
    if(DEFINED CONAN_CXX_FLAGS_${config})
      string(APPEND CMAKE_CXX_FLAGS_${config}_INIT " ${CONAN_CXX_FLAGS_${config}}")
    endif()
    if(DEFINED CONAN_C_FLAGS_${config})
      string(APPEND CMAKE_C_FLAGS_${config}_INIT " ${CONAN_C_FLAGS_${config}}")
    endif()
    if(DEFINED CONAN_SHARED_LINKER_FLAGS_${config})
      string(APPEND CMAKE_SHARED_LINKER_FLAGS_${config}_INIT " ${CONAN_SHARED_LINKER_FLAGS_${config}}")
    endif()
    if(DEFINED CONAN_EXE_LINKER_FLAGS_${config})
      string(APPEND CMAKE_EXE_LINKER_FLAGS_${config}_INIT " ${CONAN_EXE_LINKER_FLAGS_${config}}")
    endif()
endforeach()

if(DEFINED CONAN_CXX_FLAGS)
  string(APPEND CMAKE_CXX_FLAGS_INIT " ${CONAN_CXX_FLAGS}")
endif()
if(DEFINED CONAN_C_FLAGS)
  string(APPEND CMAKE_C_FLAGS_INIT " ${CONAN_C_FLAGS}")
endif()
if(DEFINED CONAN_SHARED_LINKER_FLAGS)
  string(APPEND CMAKE_SHARED_LINKER_FLAGS_INIT " ${CONAN_SHARED_LINKER_FLAGS}")
endif()
if(DEFINED CONAN_EXE_LINKER_FLAGS)
  string(APPEND CMAKE_EXE_LINKER_FLAGS_INIT " ${CONAN_EXE_LINKER_FLAGS}")
endif()

get_property( _CMAKE_IN_TRY_COMPILE GLOBAL PROPERTY IN_TRY_COMPILE )
if(_CMAKE_IN_TRY_COMPILE)
    message(STATUS "Running toolchain IN_TRY_COMPILE")
    return()
endif()

set(CMAKE_FIND_PACKAGE_PREFER_CONFIG ON)

# Definition of CMAKE_MODULE_PATH
# the generators folder (where conan generates files, like this toolchain)
list(PREPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR})

# Definition of CMAKE_PREFIX_PATH, CMAKE_XXXXX_PATH
# The Conan local "generators" folder, where this toolchain is saved.
list(PREPEND CMAKE_PREFIX_PATH ${CMAKE_CURRENT_LIST_DIR} )
list(PREPEND CMAKE_LIBRARY_PATH "C:/Users/r/.conan2/p/b/spdlo40a8ff2dd7f18/p/lib" "C:/Users/r/.conan2/p/b/fmt0eca5a1028918/p/lib" "C:/Users/r/.conan2/p/b/gtest9b400d18a4af7/p/lib")
list(PREPEND CMAKE_INCLUDE_PATH "C:/Users/r/.conan2/p/nlohm0567ffc90cfc1/p/include" "C:/Users/r/.conan2/p/geglecf7297a4deceb/p/include" "C:/Users/r/.conan2/p/cpptoc40e3c80d3642/p/include" "C:/Users/r/.conan2/p/b/spdlo40a8ff2dd7f18/p/include" "C:/Users/r/.conan2/p/b/fmt0eca5a1028918/p/include" "C:/Users/r/.conan2/p/b/gtest9b400d18a4af7/p/include")

if (DEFINED ENV{PKG_CONFIG_PATH})
set(ENV{PKG_CONFIG_PATH} "${CMAKE_CURRENT_LIST_DIR};$ENV{PKG_CONFIG_PATH}")
else()
set(ENV{PKG_CONFIG_PATH} "${CMAKE_CURRENT_LIST_DIR};")
endif()

message(STATUS "Conan toolchain: Setting BUILD_SHARED_LIBS = OFF")
set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build shared libraries")

# Variables
# Variables  per configuration

# Preprocessor definitions
# Preprocessor definitions per configuration

if(CMAKE_POLICY_DEFAULT_CMP0091)  # Avoid unused and not-initialized warnings
endif()

I also noticed that @jonathanmcdermid’s conan_toolchain.cmake file contains this line, which mine does not:

set(CONAN_RUNTIME_LIB_DIRS "C:/Users/J/.conan2/p/b/spdlobaa73156cabf6/p/bin" "C:/Users/J/.conan2/p/b/fmte745f22fd117a/p/bin" "C:/Users/J/.conan2/p/b/gtest01c84ae5757a5/p/bin" )

I’ve done my best to troubleshoot this issue, but it seems to be beyond my current knowledge of Conan and CMake.

valgur commented 1 month ago

@rldoris The CONAN_RUNTIME_LIB_DIRS variable was added fairly recently in Conan v2.4.0 (https://github.com/conan-io/conan/commit/29315889b45f24f83c6530a8bdb18b4c547e1cba), which is likely the cause of the issue you're seeing. Thanks for including the list of versions, that really helped.

I updated the minimum required Conan version to 2.4.0 in conanfile.py and conan_provider.cmake accordingly.