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

CMake V2 + Cpp20 + (Almost) Header only #1757

Merged
merged 67 commits into from
Feb 17, 2025
Merged

Conversation

BDoignies
Copy link
Contributor

PR Description

A proposal for new CMake :

  • New CMake minimum standard to 3.20 (at least 3.18 is recommended for FindPython)
  • Installation mostly managed by CMake
  • Move dependency management files to deps/folder
  • Build target per folder (instead of globbing everything into the library)
  • Removed unnecessary files

Header only changes:

  • Previous .cpp files are copied to corresponding .ih files
  • Global variables are now inline (requires C++17)
  • Board was with its .cpp files; but it would probably be a good idea to use [email protected]:c-koi/libboard.git instead
  • StdDefs are left in .cpp files for now, until their benefits for faster compilation are tested.

Standard to C++20, which has the following side effects on the code:

  • Plain old enums are now inline constexpr : C++20 depreciates addition between enums + their namespace was not used
  • Allocator for MPolynomial uses std::allocator_traits (C++20 removes most Alloc::* types...)

TODO :

  • Update documentation (if necessary) for the new CMake (at least for linking with DGtal::DGtal)
  • Validate CMake (my tests may have been too shallow, @dcoeurjo there was a problem with eigen if I am right)
  • Test Python binding to see if building still works
  • Test github actions

Sorry for the big PR, there was a bit too much cascading from what I thought were simple changes...

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug mode.
  • All continuous integration tests pass (Github Actions)

@BDoignies BDoignies requested a review from dcoeurjo January 15, 2025 14:24
Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

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

Thanks for the edits

@dcoeurjo
Copy link
Member

Before I can merge, can you please enhanced a bit the changelog?

@dcoeurjo
Copy link
Member

  • documentation ? At least the "building" module page

@BDoignies
Copy link
Contributor Author

BDoignies commented Feb 11, 2025

I enhanced a bit the changelog, is it sufficient ?

I made minor edits to moduleBuildDGtal.dox. But I do not know if anything more is needed... Everything is backward compatible; cmake commands, variable as well as C++ modules are the same.
The very few things that changed in the C++ code are:

  • The allocators, and this is compatible with C++17 which was the previous minimal standard
  • Warning fixes for examples and tests
  • Removal of .cpp files (that are not included by any code anyway)

@BDoignies
Copy link
Contributor Author

BDoignies commented Feb 11, 2025

Btw : Shall I also clean the Codacy Static Code analysis?

@BDoignies
Copy link
Contributor Author

Note: do not merge yet, there is no test / examples built by the PR. Wait a bit until I fixed DGTalTools / DGTal compilation with the new build system.

@BDoignies
Copy link
Contributor Author

Hey @dcoeurjo

I fixed the compilation with DGTalTools.

Can you check that the new GH action suits you ? It checks compilation against the PR main2.0 of DGTalTools. The last commits also changes the Install.cmake and the CheckDependancies files (there was an issue with ITK including non-existent paths...).

Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

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

All good thx!

@dcoeurjo
Copy link
Member

I will merge this one to the main2.0 branch, thx @BDoignies

@dcoeurjo
Copy link
Member

all good, merging, thx

@dcoeurjo dcoeurjo merged commit 48e76e4 into DGtal-team:main2.0 Feb 17, 2025
8 checks passed
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