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

build: Add CMake config and libraries #13

Merged
merged 11 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/code-linting-checks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,9 @@ jobs:
tar --version
task --version

- name: "Install project dependencies "
timeout-minutes: 10
continue-on-error: false
run: "task deps:lib_install"

- run: "task lint:check"
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
[submodule "tools/yscope-dev-utils"]
path = tools/yscope-dev-utils
url = https://github.com/y-scope/yscope-dev-utils.git
[submodule "submodules/Catch2"]
path = submodules/Catch2
url = https://github.com/catchorg/Catch2.git
Comment on lines +4 to +6
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Submodule initialization needs attention

The Catch2 submodule is present but appears to be missing its integration:

  • The submodule is initialized but not referenced in CMake files
  • No test files currently use the Catch2 framework
  • The submodule commit hash differs from the initialized state

Please ensure:

  • Add proper CMake integration for Catch2
  • Create test files that utilize the framework
  • Run git submodule update --init to sync the submodule
🔗 Analysis chain

LGTM! The Catch2 submodule is properly configured.

The addition follows Git submodule best practices with proper path organization and secure URL protocol.

Let's verify the submodule initialization:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Catch2 submodule initialization and version

# Test: Check if submodule is properly initialized
git submodule status submodules/Catch2

# Test: Verify Catch2 version matches the one mentioned in AI summary
cd submodules/Catch2 2>/dev/null && git rev-parse HEAD

Length of output: 185


Script:

#!/bin/bash
# Let's verify if the submodule is properly referenced in CMake files
# since the PR objectives mention adding CMake config

# Search for Catch2 references in CMake files
fd -e cmake -e txt --exec rg -l "Catch2" {}

# Check if there's a proper add_subdirectory for Catch2
rg "add_subdirectory.*Catch2" -A 2 -B 2

# Look for test files that might use Catch2
fd -e cpp -e h --exec rg -l "catch2|CATCH2" {}

Length of output: 138

96 changes: 91 additions & 5 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,96 @@ if(NOT CMAKE_BUILD_TYPE)
)
endif()

add_executable(spider)
target_compile_features(spider PRIVATE cxx_std_20)
# Add local CMake module directory to CMake's modules path
set(CMAKE_MODULE_PATH
${CMAKE_MODULE_PATH}
"${CMAKE_SOURCE_DIR}/cmake/Modules/"
)

# Macro providing the length of the absolute source directory path so we can
# create a relative (rather than absolute) __FILE__ macro
string(LENGTH "${CMAKE_SOURCE_DIR}/" SOURCE_PATH_SIZE)
add_definitions("-DSOURCE_PATH_SIZE=${SOURCE_PATH_SIZE}")

# Profiling options
add_definitions(-DPROF_ENABLED=0)

# Compile-in debug logging statements
#add_definitions(-DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG)

Comment on lines +43 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Convert hard-coded options to CMake variables

Consider making these debug and profiling options configurable through CMake variables instead of hard-coding them.

-# Profiling options
-add_definitions(-DPROF_ENABLED=0)
+# Profiling options (default: OFF)
+option(SPIDER_ENABLE_PROFILING "Enable profiling" OFF)
+add_definitions(-DPROF_ENABLED=$<BOOL:${SPIDER_ENABLE_PROFILING}>)

-# Compile-in debug logging statements
-#add_definitions(-DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG)
+# Debug logging (default: OFF)
+option(SPIDER_ENABLE_DEBUG_LOGGING "Enable debug logging" OFF)
+if(SPIDER_ENABLE_DEBUG_LOGGING)
+    add_definitions(-DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG)
+endif()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Profiling options
add_definitions(-DPROF_ENABLED=0)
# Compile-in debug logging statements
#add_definitions(-DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG)
# Profiling options (default: OFF)
option(SPIDER_ENABLE_PROFILING "Enable profiling" OFF)
add_definitions(-DPROF_ENABLED=$<BOOL:${SPIDER_ENABLE_PROFILING}>)
# Debug logging (default: OFF)
option(SPIDER_ENABLE_DEBUG_LOGGING "Enable debug logging" OFF)
if(SPIDER_ENABLE_DEBUG_LOGGING)
add_definitions(-DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG)
endif()

# Flush to disk switch
add_definitions(-DFLUSH_TO_DISK_ENABLED=1)

# Make off_t 64-bit
add_definitions(-D_FILE_OFFSET_BITS=64)

# Detect linking mode (static or shared); Default to static.
set(SPIDER_USE_STATIC_LIBS ON CACHE BOOL "Whether to link against static libraries")
if(SPIDER_USE_STATIC_LIBS)
if(APPLE)
set(SPIDER_STATIC_LIBS_UNSUPPORTED_PLATFORM "macOS")
elseif(EXISTS "/etc/centos-release")
set(SPIDER_STATIC_LIBS_UNSUPPORTED_PLATFORM "CentOS")
endif()

if(DEFINED SPIDER_STATIC_LIBS_UNSUPPORTED_PLATFORM)
message(
AUTHOR_WARNING
"Building with static libraries is unsupported on"
" ${SPIDER_STATIC_LIBS_UNSUPPORTED_PLATFORM}. Switching to shared libraries."
)
set(SPIDER_USE_STATIC_LIBS OFF)
endif()
endif()
if(SPIDER_USE_STATIC_LIBS)
set(SPIDER_LIBS_STRING "static")
else()
set(SPIDER_LIBS_STRING "shared")
endif()
message(STATUS "Building using ${SPIDER_LIBS_STRING} libraries")

# Find and setup fmt
find_package(fmt 8.0.1 REQUIRED)
if(fmt_FOUND)
message(STATUS "Found fmt ${fmt_VERSION}")
else()
message(FATAL_ERROR "Could not find static libraries for fmt")
endif()

# Find and setup spdlog
if(SPIDER_USE_STATIC_LIBS)
# NOTE: On some Linux distributions (e.g. Ubuntu), the spdlog package only contains a dynamic
# library. If the `find_package(spdlog)` call below fails, re-run
# `tools/scripts/lib_install/<dist_name>/install-packages-from-source.sh` to build spdlog from
# source.
set(spdlog_USE_STATIC_LIBS ON)
endif()
set(SPDLOG_FMT_EXTERNAL ON)
find_package(spdlog 1.9.2 REQUIRED)
if(spdlog_FOUND)
message(STATUS "Found spdlog ${spdlog_VERSION}")
else()
if(SPIDER_USE_STATIC_LIBS)
message(FATAL_ERROR "Could not find static libraries for spdlog.`")
else()
message(FATAL_ERROR "Could not find libraries for spdlog.")
endif()
endif()

# Find and setup MariaDBClient library
if(CLP_USE_STATIC_LIBS)
# NOTE: We can't statically link to MariaDBClient since it's GPL
message(AUTHOR_WARNING "MariaDBClient cannot be statically linked due to its license.")
endif()
find_package(MariaDBClient 3.1.0 REQUIRED)
if(MariaDBClient_FOUND)
message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}")
else()
message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient")
endif()
Comment on lines +109 to +118
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect variable name in MariaDBClient configuration

There's a variable name inconsistency that needs to be corrected.

-if(CLP_USE_STATIC_LIBS)
+if(SPIDER_USE_STATIC_LIBS)
     # NOTE: We can't statically link to MariaDBClient since it's GPL
     message(AUTHOR_WARNING "MariaDBClient cannot be statically linked due to its license.")
 endif()
 find_package(MariaDBClient 3.1.0 REQUIRED)
 if(MariaDBClient_FOUND)
     message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}")
 else()
-    message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient")
+    message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for MariaDBClient")
 endif()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if(CLP_USE_STATIC_LIBS)
# NOTE: We can't statically link to MariaDBClient since it's GPL
message(AUTHOR_WARNING "MariaDBClient cannot be statically linked due to its license.")
endif()
find_package(MariaDBClient 3.1.0 REQUIRED)
if(MariaDBClient_FOUND)
message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}")
else()
message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient")
endif()
if(SPIDER_USE_STATIC_LIBS)
# NOTE: We can't statically link to MariaDBClient since it's GPL
message(AUTHOR_WARNING "MariaDBClient cannot be statically linked due to its license.")
endif()
find_package(MariaDBClient 3.1.0 REQUIRED)
if(MariaDBClient_FOUND)
message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}")
else()
message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for MariaDBClient")
endif()


find_package(Threads REQUIRED)

set(SPIDER_SOURCES src/spider/spider.cpp)
target_sources(spider PRIVATE ${SPIDER_SOURCES})
add_subdirectory(src/spider)

target_include_directories(spider PRIVATE src/)
add_subdirectory(tests)
59 changes: 59 additions & 0 deletions cmake/Modules/FindLibraryDependencies.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Find the given libraries ignoring libraries that should be dynamically linked
# Params:
# fld_LIBNAME - Name of the library (without the 'lib' prefix)
# fld_PREFIX - Prefix for created variables
# fld_STATIC_LIBS - List of libraries to find
# Returns:
# ${fld_LIBNAME}_DYNAMIC_LIBS - List of libraries that should be dynamically linked
# ${fld_PREFIX}_LIBRARY_DEPENDENCIES - Found libraries
macro(FindStaticLibraryDependencies fld_LIBNAME fld_PREFIX fld_STATIC_LIBS)
# NOTE: libc, libm, libpthread, and librt should be dynamically linked
set(fld_DYNAMIC_LIBS "c;m;pthread;rt")

# Get absolute path of dependent libraries
foreach(fld_DEP_LIB ${fld_STATIC_LIBS})
# Skip dynamic libs
set(fld_IGNORE_LIB FALSE)
foreach(fld_DYNAMIC_LIB ${fld_DYNAMIC_LIBS})
if(${fld_DEP_LIB} STREQUAL ${fld_DYNAMIC_LIB})
set(fld_IGNORE_LIB TRUE)
list(APPEND ${fld_PREFIX}_DYNAMIC_LIBS "${fld_DEP_LIB}")
endif()
endforeach()
if(fld_IGNORE_LIB OR ${fld_DEP_LIB} STREQUAL ${fld_LIBNAME})
continue()
endif()

find_library(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY NAMES ${fld_DEP_LIB} PATH_SUFFIXES lib)
if(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY)
list(
APPEND
${fld_PREFIX}_LIBRARY_DEPENDENCIES
"${${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY}"
)
else()
message(SEND_ERROR "Static ${fld_DEP_LIB} library not found")
endif()
endforeach()
endmacro()

# Find the given libraries
# Params:
# fld_PREFIX - Prefix for created variables
# fld_DYNAMIC_LIBS - List of libraries to find
# Returns:
# ${fld_PREFIX}_LIBRARY_DEPENDENCIES - Found libraries
macro(FindDynamicLibraryDependencies fld_PREFIX fld_DYNAMIC_LIBS)
foreach(fld_DEP_LIB ${fld_DYNAMIC_LIBS})
find_library(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY NAMES ${fld_DEP_LIB} PATH_SUFFIXES lib)
if(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY)
list(
APPEND
${fld_PREFIX}_LIBRARY_DEPENDENCIES
"${${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY}"
)
else()
message(SEND_ERROR "${fld_DEP_LIB} library not found")
endif()
endforeach()
endmacro()
123 changes: 123 additions & 0 deletions cmake/Modules/FindMariaDBClient.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
# Try to find MariaDBClient
# NOTE: The FindMariaDBClient.cmake included with CMake has no support for static libraries, so we use our own.
#
# Set MariaDBClient_USE_STATIC_LIBS=ON to look for static libraries.
#
# Once done this will define:
# MariaDBClient_FOUND - Whether MariaDBClient was found on the system
# MariaDBClient_INCLUDE_DIR - The MariaDBClient include directories
# MariaDBClient_VERSION - The version of MariaDBClient installed on the system
#
# Conventions:
# - Variables only for use within the script are prefixed with "mariadbclient_"
# - Variables that should be externally visible are prefixed with "MariaDBClient_"

set(mariadbclient_LIBNAME "mariadb")

include(cmake/Modules/FindLibraryDependencies.cmake)

# Run pkg-config
find_package(PkgConfig)
pkg_check_modules(mariadbclient_PKGCONF QUIET "lib${mariadbclient_LIBNAME}")

# Set include directory
find_path(
MariaDBClient_INCLUDE_DIR
mysql.h
HINTS
${mariadbclient_PKGCONF_INCLUDEDIR}
PATH_SUFFIXES
mariadb
)
Comment on lines +24 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for pkg-config results

Consider validating the pkg-config results before proceeding with the path search. This would provide clearer error messages when the library isn't properly configured in the system.

 # Set include directory
+if(NOT mariadbclient_PKGCONF_FOUND)
+    message(DEBUG "pkg-config could not find lib${mariadbclient_LIBNAME}")
+endif()
+
 find_path(
     MariaDBClient_INCLUDE_DIR
     mysql.h
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
find_path(
MariaDBClient_INCLUDE_DIR
mysql.h
HINTS
${mariadbclient_PKGCONF_INCLUDEDIR}
PATH_SUFFIXES
mariadb
)
if(NOT mariadbclient_PKGCONF_FOUND)
message(DEBUG "pkg-config could not find lib${mariadbclient_LIBNAME}")
endif()
find_path(
MariaDBClient_INCLUDE_DIR
mysql.h
HINTS
${mariadbclient_PKGCONF_INCLUDEDIR}
PATH_SUFFIXES
mariadb
)


# Handle static libraries
if(MariaDBClient_USE_STATIC_LIBS)
# Save current value of CMAKE_FIND_LIBRARY_SUFFIXES
set(mariadbclient_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})

# Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix
set(CMAKE_FIND_LIBRARY_SUFFIXES .a)
endif()
Comment on lines +38 to +40
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider platform-specific static library suffixes

The current implementation only handles .a files, which might not work on all platforms. Consider using CMAKE_STATIC_LIBRARY_SUFFIX instead.

-    set(CMAKE_FIND_LIBRARY_SUFFIXES .a)
+    set(CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_STATIC_LIBRARY_SUFFIX})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix
set(CMAKE_FIND_LIBRARY_SUFFIXES .a)
endif()
# Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix
set(CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_STATIC_LIBRARY_SUFFIX})
endif()


# Find library
find_library(
MariaDBClient_LIBRARY
NAMES
${mariadbclient_LIBNAME}
HINTS
${mariadbclient_PKGCONF_LIBDIR}
PATH_SUFFIXES
lib
)
if(MariaDBClient_LIBRARY)
# NOTE: This must be set for find_package_handle_standard_args to work
set(MariaDBClient_FOUND ON)
endif()

if(MariaDBClient_USE_STATIC_LIBS)
findstaticlibrarydependencies(${mariadbclient_LIBNAME} mariadbclient
"${mariadbclient_PKGCONF_STATIC_LIBRARIES}"
)

# Restore original value of CMAKE_FIND_LIBRARY_SUFFIXES
set(CMAKE_FIND_LIBRARY_SUFFIXES ${mariadbclient_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES})
unset(mariadbclient_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES)
endif()

finddynamiclibrarydependencies(mariadbclient "${mariadbclient_DYNAMIC_LIBS}")

# Set version
set(MariaDBClient_VERSION ${mariadbclient_PKGCONF_VERSION})

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(
MariaDBClient
REQUIRED_VARS
MariaDBClient_INCLUDE_DIR
VERSION_VAR MariaDBClient_VERSION
)

if(NOT TARGET MariaDBClient::MariaDBClient)
# Add library to build
if(MariaDBClient_FOUND)
if(MariaDBClient_USE_STATIC_LIBS)
add_library(MariaDBClient::MariaDBClient STATIC IMPORTED)
else()
# NOTE: We use UNKNOWN so that if the user doesn't have the SHARED
# libraries installed, we can still use the STATIC libraries
add_library(MariaDBClient::MariaDBClient UNKNOWN IMPORTED)
endif()
endif()
Comment on lines +82 to +90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add IMPORTED_SONAME property for shared libraries

When using shared libraries, it's important to set the IMPORTED_SONAME property to ensure proper runtime linking.

         else()
             # NOTE: We use UNKNOWN so that if the user doesn't have the SHARED
             # libraries installed, we can still use the STATIC libraries
             add_library(MariaDBClient::MariaDBClient UNKNOWN IMPORTED)
+            if(NOT MariaDBClient_USE_STATIC_LIBS)
+                get_filename_component(_soname "${MariaDBClient_LIBRARY}" NAME)
+                set_target_properties(MariaDBClient::MariaDBClient
+                    PROPERTIES
+                        IMPORTED_SONAME "${_soname}")
+            endif()
         endif()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if(MariaDBClient_FOUND)
if(MariaDBClient_USE_STATIC_LIBS)
add_library(MariaDBClient::MariaDBClient STATIC IMPORTED)
else()
# NOTE: We use UNKNOWN so that if the user doesn't have the SHARED
# libraries installed, we can still use the STATIC libraries
add_library(MariaDBClient::MariaDBClient UNKNOWN IMPORTED)
endif()
endif()
if(MariaDBClient_FOUND)
if(MariaDBClient_USE_STATIC_LIBS)
add_library(MariaDBClient::MariaDBClient STATIC IMPORTED)
else()
# NOTE: We use UNKNOWN so that if the user doesn't have the SHARED
# libraries installed, we can still use the STATIC libraries
add_library(MariaDBClient::MariaDBClient UNKNOWN IMPORTED)
if(NOT MariaDBClient_USE_STATIC_LIBS)
get_filename_component(_soname "${MariaDBClient_LIBRARY}" NAME)
set_target_properties(MariaDBClient::MariaDBClient
PROPERTIES
IMPORTED_SONAME "${_soname}")
endif()
endif()
endif()


# Set include directories for library
if(MariaDBClient_INCLUDE_DIR)
set_target_properties(
MariaDBClient::MariaDBClient
PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES
"${MariaDBClient_INCLUDE_DIR}"
)
endif()

# Set location of library
if(EXISTS "${MariaDBClient_LIBRARY}")
set_target_properties(
MariaDBClient::MariaDBClient
PROPERTIES
IMPORTED_LINK_INTERFACE_LANGUAGES
"C"
IMPORTED_LOCATION
"${MariaDBClient_LIBRARY}"
)

# Add component's dependencies for linking
if(mariadbclient_LIBRARY_DEPENDENCIES)
set_target_properties(
MariaDBClient::MariaDBClient
PROPERTIES
INTERFACE_LINK_LIBRARIES
"${mariadbclient_LIBRARY_DEPENDENCIES}"
)
endif()
endif()
endif()
Loading
Loading