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

Implement CMake project #1

Merged
merged 21 commits into from
Nov 8, 2023
Merged

Implement CMake project #1

merged 21 commits into from
Nov 8, 2023

Conversation

LecrisUT
Copy link
Owner

@LecrisUT LecrisUT commented Nov 6, 2023

No description provided.

This comment was marked as outdated.

@LecrisUT

This comment was marked as off-topic.

@LecrisUT
Copy link
Owner Author

LecrisUT commented Nov 6, 2023

So there is a lot in this PR, but I will add some comments in the significant parts of the code to highlight a few paradimes

CMakeLists.txt Show resolved Hide resolved
Comment on lines +46 to +48
option(TEMPLATE_TESTS "Template: Build test-suite" ${PROJECT_IS_TOP_LEVEL})
option(TEMPLATE_SHARED_LIBS "Template: Build as a shared library" ${PROJECT_IS_TOP_LEVEL})
option(TEMPLATE_INSTALL "Template: Install project" ${PROJECT_IS_TOP_LEVEL})
Copy link
Owner Author

Choose a reason for hiding this comment

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

Another common default I use are these 3 options with the defaults set according to if the project is included as FetchContent or built as standalone. I believe SHARED_LIBS is a sensible option here because downstream projects should avoid linking as shared library and potentially being overwritten by the system installed libraries, unless they know what they are doing

Comment on lines +76 to +82
FetchContent_Declare(fmt
GIT_REPOSITORY https://github.com/fmtlib/fmt
GIT_TAG master
FIND_PACKAGE_ARGS MODULE
)

FetchContent_MakeAvailable(fmt)
Copy link
Owner Author

Choose a reason for hiding this comment

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

With FIND_PACKAGE_ARGS, FetchContent is the most flexible way of defining the external dependencies since it can be controlled via CMAKE_REQUIRE_FIND_PACKAGE_<PackageName> and CMAKE_DISABLE_FIND_PACKAGE_<PackageName>. Also see Findfmt.cmake for further implementation.

Comment on lines +12 to +24
Template_FindPackage(${CMAKE_FIND_PACKAGE_NAME}
NAMES fmt
PKG_MODULE_NAMES fmt
)

# Create appropriate aliases
if (${CMAKE_FIND_PACKAGE_NAME}_PKGCONFIG)
add_library(fmt::fmt ALIAS PkgConfig::${CMAKE_FIND_PACKAGE_NAME})
endif ()
set_package_properties(${CMAKE_FIND_PACKAGE_NAME} PROPERTIES
URL https://github.com/fmtlib/fmt
DESCRIPTION "A modern formatting library "
)
Copy link
Owner Author

Choose a reason for hiding this comment

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

The goal of this Findfmt.cmake module is to make a unified interface of fmt::fmt regardless of if it is imported via FetchContent (outside of this file), or find_package(CONFIG) or pkg-config (handled in Template_FindPackage). This could also be expanded to handled un-packaged libraries, but that has to be defined manually

CMakeLists.txt Show resolved Hide resolved
Comment on lines 1 to 49
project(Template_test)

set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS ON)

set(external_libs)
if (NOT Template_IS_TOP_LEVEL)
FetchContent_Declare(Template
GIT_REPOSITORY https://github.com/LecrisUT/CMake-Template
GIT_TAG main
FIND_PACKAGE_ARGS CONFIG
)
list(APPEND external_libs Template)
endif ()
Copy link
Owner Author

Choose a reason for hiding this comment

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

This structure I think it is good to advertise. This allows for importing and running the tests individually from an external testing suite like tmt or from a cross-compiled environment like in conda.

test/CMakeLists.txt Show resolved Hide resolved
Comment on lines +5 to +11
set(test_list
FetchContent
find_package
)
if (NOT Template_IS_TOP_LEVEL)
list(APPEND test_list pkg-config)
endif ()
Copy link
Owner Author

Choose a reason for hiding this comment

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

Packaging tests, also callable from tmt. pkg-config I don't think can be easily tested outside of a tmt environment because of the configure_file format.

.packit.yaml Outdated
Comment on lines 30 to 46
- job: copr_build
trigger: pull_request
owner: lecris
project: cmake-template
targets:
- fedora-all-x86_64
- fedora-all-aarch64
- epel-9-x86_64
- epel-9-aarch64
- job: tests
trigger: pull_request
targets:
- fedora-all-x86_64
- fedora-all-aarch64
- epel-9-x86_64
- epel-9-aarch64
fmf_path: .distro
Copy link
Owner Author

Choose a reason for hiding this comment

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

Fedora packaging with tests :)

@@ -0,0 +1,29 @@
repos:
Copy link
Owner Author

Choose a reason for hiding this comment

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

Common pre-commits, maybe can include more/better tools?

@LecrisUT LecrisUT mentioned this pull request Nov 6, 2023
@github-advanced-security

This comment was marked as off-topic.

Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT LecrisUT force-pushed the cmake/structure branch 6 times, most recently from 380198f to 6a769e4 Compare November 7, 2023 16:12
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@LecrisUT LecrisUT force-pushed the cmake/structure branch 3 times, most recently from 3da388d to 52d7a26 Compare November 7, 2023 17:02
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT LecrisUT changed the title Implement Implement CMake project Nov 7, 2023
@LecrisUT LecrisUT force-pushed the cmake/structure branch 3 times, most recently from 4f31eca to 6b0c40b Compare November 7, 2023 19:10
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Catch2 version is not compatible and backporting FetchContent(FIND_PACKAGE_ARGS) from CMake 3.24 is a hustle with nested projects

Signed-off-by: Cristian Le <[email protected]>
Copy link

sonarqubecloud bot commented Nov 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@LecrisUT LecrisUT merged commit cee25b2 into main Nov 8, 2023
16 of 19 checks passed
@LecrisUT LecrisUT deleted the cmake/structure branch December 15, 2023 20:47
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

Successfully merging this pull request may close these issues.

2 participants