Skip to content

Conversation

@jmacheta
Copy link
Member

add modern tooling and runtime version information for this component

@embetech-official embetech-official deleted a comment from sonarqubecloud bot Dec 10, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds comprehensive versioning infrastructure to the CRC component library, including runtime version query APIs, modern CMake tooling with CPM package manager, improved build configuration, and updated CI/CD workflows.

  • Introduces version query functions CRC_GetVersion() and CRC_GetVersionString() with semantic versioning support
  • Migrates from FetchContent to CPM.cmake for dependency management (embeutils, googletest)
  • Refactors CMake build system with FILE_SET headers, improved installation, and Git-based commit tracking

Reviewed changes

Copilot reviewed 22 out of 24 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
VERSION.txt New file containing the semantic version number (2.0.1)
src/include/embetech/crc.h New umbrella header providing version query API
src/crc_version.c Implementation of version query functions with compile-time definitions
src/CMakeLists.txt Refactored build configuration with FILE_SET headers, version definitions from Git, and dependency on embetech::utils
CMakeLists.txt Updated to read version from VERSION.txt file
cmake/requirements.cmake New file introducing CPM package manager for dependencies
cmake/crc-config_template.cmake Simplified package config template for installed library
cmake/install.cmake Streamlined installation logic using FILE_SET
cmake/install_header_licenses.cmake New install-time script for header metadata substitution
tests/CMakeLists.txt Updated to use CPM-provided GoogleTest with workaround for compiler warnings
tests/crc_ut.cpp Simplified includes to use new umbrella header
doc/CMakeLists.txt Made Doxygen optional and added ALL flag to documentation target
README.md Updated with CPM-based installation instructions and corrected typos
CMakePresets.json Updated presets for GCC toolchain with workflow support
.github/workflows/on_push.yml New simplified CI workflow using CMake workflow presets
.github/workflows/generate_artifacts.yml New multi-architecture artifact generation workflow
LICENSE.md New MIT license file
.cspell.yml New spell-checking configuration
.clangd New clangd LSP configuration
.cmake-format Updated with CPMAddPackage command formatting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 11 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 21 to 22
#if (!defined CRC_COMMIT_ID) || (0 == AS_HEX(CRC_COMMIT_ID))
#define CRC_COMMIT_ID_STR "local"
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The preprocessor condition (!defined CRC_COMMIT_ID) || (0 == AS_HEX(CRC_COMMIT_ID)) has a potential issue. If CRC_COMMIT_ID is not defined, the macro expansion AS_HEX(CRC_COMMIT_ID) will attempt to create the token 0x (empty), which is invalid and will cause a compilation error. The || operator in preprocessor directives does not short-circuit macro expansion. Consider restructuring this as separate #ifndef and #if directives, or ensure that CRC_COMMIT_ID is always defined (even if empty) before this check.

Suggested change
#if (!defined CRC_COMMIT_ID) || (0 == AS_HEX(CRC_COMMIT_ID))
#define CRC_COMMIT_ID_STR "local"
#ifndef CRC_COMMIT_ID
#define CRC_COMMIT_ID_STR "local"
#elif (0 == AS_HEX(CRC_COMMIT_ID))
#define CRC_COMMIT_ID_STR "local"

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,12 @@
set(INSTALL_DIR $ENV{DESTDIR}${CMAKE_INSTALL_PREFIX})

file(STRINGS ${CMAKE_CURRENT_LIST_DIR}/../VERSION.txt PROJECT_VERSION)
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

This script reads VERSION.txt using a relative path ${CMAKE_CURRENT_LIST_DIR}/../VERSION.txt. When executed via install(SCRIPT ...), this assumes the source tree is available at install time in the same location. This may fail in scenarios where the installation is performed from a binary package or after the source directory has been moved. Consider passing PROJECT_VERSION as a variable to the script using install(SCRIPT ... CODE "set(PROJECT_VERSION ...)") or using configure_file to create a script with the version baked in.

Suggested change
file(STRINGS ${CMAKE_CURRENT_LIST_DIR}/../VERSION.txt PROJECT_VERSION)
# PROJECT_VERSION should be set externally (e.g., via install(SCRIPT ... CODE "set(PROJECT_VERSION ...)"))

Copilot uses AI. Check for mistakes.
#define CRC_COMMIT_ID_STR STR(CRC_COMMIT_ID)
#endif

#define CRC_VERSION STR(CRC_VERSION_MAJOR) "." STR(CRC_VERSION_MINOR) "." STR(CRC_VERSION_PATCH) "-" CRC_COMMIT_ID_STR
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The version string format uses a hyphen separator before the commit ID (e.g., "2.0.1-abc123" or "2.0.1-local"), which is not standard semantic versioning. In semantic versioning, pre-release identifiers use a hyphen, but build metadata uses a plus sign. Since the commit ID is build metadata rather than a pre-release identifier, consider using "+" instead (e.g., "2.0.1+abc123"). Alternatively, document this deviation from semantic versioning.

Suggested change
#define CRC_VERSION STR(CRC_VERSION_MAJOR) "." STR(CRC_VERSION_MINOR) "." STR(CRC_VERSION_PATCH) "-" CRC_COMMIT_ID_STR
#define CRC_VERSION STR(CRC_VERSION_MAJOR) "." STR(CRC_VERSION_MINOR) "." STR(CRC_VERSION_PATCH) "+" CRC_COMMIT_ID_STR

Copilot uses AI. Check for mistakes.
/**
* @brief Get library version as a NUL-terminated string.
*
* @return Pointer to a read-only, statically-allocated string in semantic version format (e.g., "1.2.3"). Must not be freed or modified.
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The documentation states the version string is in semantic version format (e.g., "1.2.3"), but based on the implementation in src/crc_version.c line 27, the actual format is "MAJOR.MINOR.PATCH-COMMIT_ID" (e.g., "2.0.1-abc123" or "2.0.1-local"). Update the documentation to reflect the actual format including the commit ID suffix.

Suggested change
* @return Pointer to a read-only, statically-allocated string in semantic version format (e.g., "1.2.3"). Must not be freed or modified.
* @return Pointer to a read-only, statically-allocated string in the format "MAJOR.MINOR.PATCH-COMMIT_ID" (e.g., "2.0.1-abc123" or "2.0.1-local"). Must not be freed or modified.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 30 out of 32 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jmacheta and others added 2 commits December 10, 2025 20:06
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@jmacheta jmacheta merged commit 0223634 into main Dec 10, 2025
2 checks passed
@jmacheta jmacheta deleted the add_versioning branch December 10, 2025 19:10
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