Skip to content

CXX-2753 Refactor CMake config files to reflect new directory structure #1037

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

Merged
merged 12 commits into from
Oct 19, 2023

Conversation

eramongodb
Copy link
Contributor

Summary

This PR is a followup to #1026 and is related to CXX-2753. Verified by this patch.

This PR refactors the CMake config files to better reflect and take advantage of the new directory structure by moving variables and commands into the scope of per-project subdirectories as much as possible (cmake, include, lib, test). This PR also takes this opportunity to apply a consistent formatting to the modified CMake config files (whitespacing, indentation, etc.).

Due to the already large diff in this PR, the relocation of currently out-of-place test files (under the test_util directory, i.e. catch.hh) is deferred to a followup PR.

This PR does/should NOT affect binary or source compatibility (verified by this patch).

Notable Changes and Patterns

  • if(1) + endif() is used to "scope" related commands together for readability purposes.
  • Fixed redundant/incorrect regeneration of some generated files. Commands for every file that should be generated once is now only run once (e.g. export header, package targets file, etc.).
  • Globbing of source files has been replaced with explicit, sorted lists to abide by CMake recommendations.
  • pkg-config config files were updated to use CMAKE_INSTALL_INCLUDEDIR instead of assuming /include.
  • Relocation of test targets into test subdirectory also relocates their output files, which required updating the EVG config to include the test subdirectories in library/executable lookup.
  • <name>_test_properties targets are used to define and set common target properties for tests.
  • All CMake message() commands without a mode (aka message type) have been given STATUS mode, as otherwise their output is written to stderr (as-if NOTICE mode) which seems undesirable (status quo).
  • Replaced generator-specific parallelism flags with CMAKE_BUILD_PARALLEL_LEVEL.

@eramongodb eramongodb self-assigned this Oct 9, 2023
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

The reorganization and improvements are appreciated. LGTM with minor comments.

What tool was used to format the cmake files?

Aside: I like the if(1) / endif() pattern to separate blocks. When the C++ driver minimum CMake is increased to 3.25 or higher, the block command may help.

@eramongodb
Copy link
Contributor Author

What tool was used to format the cmake files?

I am using the CMake Language Support extension for VS Code.

Note: it supports a cmake.format.spaceAfterCommandName parameter if we want to preserve the space before opening parenthesis (e.g. find_package (bson-1.0 REQUIRED)). The changes in this PR use the default value of false (e.g. find_package(bson-1.0 REQUIRED)). There are a mix of both styles in the config files, so if the space is preferred, that can be applied instead.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

Sorry to delay. The CMake formatting changes are +1 good.

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.

3 participants