Skip to content

modernize build with proper install/export and metadata#24

Merged
X1aomu merged 2 commits into
zeus-cpp:mainfrom
ComixHe:main
Jun 24, 2026
Merged

modernize build with proper install/export and metadata#24
X1aomu merged 2 commits into
zeus-cpp:mainfrom
ComixHe:main

Conversation

@ComixHe

@ComixHe ComixHe commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

The library currently lacks install and find_package() support, making it difficult to consume via CMake's standard package management. Downstream projects must manually set up include directories, and there is no version metadata for dependency resolution.

@X1aomu

X1aomu commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Hi there! Thank you so much for putting together this PR and for contributing to the project. We really appreciate your time and effort! This PR packs quite a few changes, so I've broken down my thoughts into a few categories:

👍 What I agree with & find necessary

  • Adding the PROJECT_IS_TOP_LEVEL guard.
  • Enabling install(...) and BUILD_TESTING on-demand.
  • The new approach to exporting CMake targets.
  • Completing the project information.

👌 What looks fine / harmless to me

  • Linking with Catch2WithMain.
  • Setting target_compile_features(expected INTERFACE cxx_std_17).

🤔 Concerns & Suggested Adjustments

  • Target Renaming: While I completely agree with providing a namespaced alias like zeus::expected, I would prefer not to rename the main underlying target from zeus_expected to expected. The name expected is quite generic and could easily cause naming collisions or pollution in dependent projects. For instance, it could conflict when users pull it in via FetchContent:
    FetchContent_Declare(expected                              
        GIT_REPOSITORY https://github.com/zeus-cpp/expected.git
        GIT_TAG v1.3.3)                                        
    FetchContent_MakeAvailable(expected)                       
    target_link_libraries(my_app zeus::expected)

📉 Not recommended / Non-best practices

  • Replacing project(...) with set(TEST_NAME ...) in tests: While it might seem like it removes boilerplate, removing project() calls strips away the benefits of CMake's native sub-project scoping. project() automatically sets up useful variables (like <PROJECT-NAME>_SOURCE_DIR) and helps IDEs (like CLion or Visual Studio) properly group and organize test targets in the workspace tree. I'd prefer we keep the project() calls.
  • Adding sources directly in add_executable / add_library: Specifying sources directly within these commands is often considered an anti-pattern in modern CMake. The recommended best practice is to declare the target first, and then add the source files separately using target_sources.

💡 General Suggestions moving forward

  • Commit granularity: Since this PR touches many different things, could you please split these changes into smaller, atomic commits? A good rule of thumb is "one logical change per commit." This makes reviewing and maintaining the git history much easier.
  • Context and Motivation: It would be super helpful to add a bit more context explaining the intent and necessity of the changes. For example, what specific problems were you running into in your environment that prompted these changes?

Let me know what you think! Thanks again for your contribution.

The library could not be discovered via find_package() or installed,
making it hard to integrate into real projects. Add full install
pipeline (target export, Config.cmake, ConfigVersion.cmake) gated
behind PROJECT_IS_TOP_LEVEL, so it works as both a standalone project
and a FetchContent subdirectory without side effects.
@ComixHe ComixHe changed the title build(cmake): modernize build with proper install/export and metadata modernize build with proper install/export and metadata Jun 24, 2026
@ComixHe

ComixHe commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Please take another look when you have a moment. Thanks.

@X1aomu X1aomu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, almost.

Comment thread tests/test_expected/CMakeLists.txt Outdated
- Remove the custom test main boilerplate in favor of
Catch2::Catch2WithMain.
- Add LANGUAGES CXX and PRIVATE linkage
visibility to all test targets for correctness.
@X1aomu X1aomu merged commit d5af6e2 into zeus-cpp:main Jun 24, 2026
26 checks passed
@X1aomu

X1aomu commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@ComixHe Should we bump the version?

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