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

Initial CMake support #929

Merged
merged 54 commits into from
Feb 21, 2024
Merged

Initial CMake support #929

merged 54 commits into from
Feb 21, 2024

Conversation

dutkalex
Copy link
Contributor

@dutkalex dutkalex commented Feb 5, 2024

Describe your changes here:
This PR is a MVP candidate for CMake build support in t8code. Currently supports building the main library, the tests, the examples and the tutorials.

All these boxes must be checked by the reviewers before merging the pull request:

As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.

General

  • The reviewer executed the new code features at least once and checked the results manually

  • The code follows the t8code coding guidelines

  • New source/header files are properly added to the Makefiles

  • The code is well documented

  • All function declarations, structs/classes and their members have a proper doxygen documentation

  • All new algorithms and data structures are sufficiently optimal in terms of memory and runtime (If this should be merged, but there is still potential for optimization, create a new issue)

Tests

  • The code is covered in an existing or new test case using Google Test

Github action

  • The code compiles without warning in debugging and release mode, with and without MPI (this should be executed automatically in a github action)

  • All tests pass (in various configurations, this should be executed automatically in a github action)

    If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):

    • Should this use case be added to the github action?
    • If not, does the specific use case compile and all tests pass (check manually)

Scripts and Wiki

  • If a new directory with source-files is added, it must be covered by the script/find_all_source_files.scp to check the indentation of these files.
  • If this PR introduces a new feature, it must be covered in an example/tutorial and a Wiki article.

Licence

  • The author added a BSD statement to doc/ (or already has one)

@dutkalex dutkalex mentioned this pull request Feb 5, 2024
Copy link
Collaborator

@jmark jmark left a comment

Choose a reason for hiding this comment

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

Really nice work! Thank you very much! Looks very promising so far!

Would you please substitute all leafs with leaves and also merge in the latest main? We just made this global change in t8code. Some files got renamed by this.

CMakeLists.txt Outdated Show resolved Hide resolved
@dutkalex
Copy link
Contributor Author

dutkalex commented Feb 6, 2024

When building the 116 unit tests, I noticed that not all of them pass. From my limited understanding of the code base, I think that these failing tests are inherited from p4est and sc, but 2 that I ported myself fail. However, the pipeline passes as it uses the autotools BS...

@dutkalex dutkalex marked this pull request as draft February 6, 2024 10:41
@dutkalex dutkalex marked this pull request as ready for review February 6, 2024 15:08
@jmark
Copy link
Collaborator

jmark commented Feb 6, 2024

@dutkalex I could successfully build t8code with your cmake scripts. Nice work!

Indeed, not all tests pass on my machine, too!

15:test_order
16:test_complete_subtree
21:test_balance2
22:test_partition_corr2
24:test_balance_type2
25:test_lnodes2
26:test_plex2
28:test_search2
29:test_subcomm2
31:test_ghost2
32:test_iterate2
34:test_partition2
36:test_valid2
38:test_wrap2
40:test_loadsave2
41:test_all6
46:test_balance3
47:test_partition_corr3
49:test_balance_type3
50:test_lnodes3
51:test_plex3
53:test_subcomm3
55:test_ghost3
56:test_iterate3
58:test_partition3
60:test_valid3
62:test_wrap3
70:t8_gtest_version
79:t8_gtest_cmesh_set_join_by_vertices

The following error message sprang into my eye skimming through the output log of the failed tests:
[libsc 0] Abort: Configure did not find a recent enough zlib. Abort.

Also some mesh files are not found.
[p4est 0] Failed to open test/testfiles/test_cube_unstructured_1.inp
Here, cmake probably has to copy over some data resources as well.

@dutkalex
Copy link
Contributor Author

dutkalex commented Feb 6, 2024

The zlib part is managed by the existing p4est & sc CMake build system which does strange things at times on my machine too. Maybe I will try to fix this in the future but it is outside of the scope of this PR IMO. I will investigate the 2 t8_gtest_*** which fail and this mesh file thing. Thx for the feedback! 🙃

EDIT: The missing mesh stuff is also on the p4est side it seems...

Copy link
Collaborator

@jmark jmark 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 you effort! Very educational reading you cmake scripts!

It would be nice if we could replicate the folder structure in the INSTALL_PREFIX as done with the old autotools build system. For example, we have bindings generators for e.g. Julia which assume this structure.

Let me know if you need more info!

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
example/CMakeLists.txt Outdated Show resolved Hide resolved
@dutkalex
Copy link
Contributor Author

dutkalex commented Feb 6, 2024

It would be nice if we could replicate the folder structure in the INSTALL_PREFIX as done with the old autotools build system. For example, we have bindings generators for e.g. Julia which assume this structure.

@jmark What install directory structure is needed exactly? As of this comment it just contains lib/libt8.* and include/some_zlib_stuff

@dutkalex
Copy link
Contributor Author

dutkalex commented Feb 7, 2024

Also, what is your policy regarding documentation? Where do you want me to document how to use the CMake BS? In a new section in the INSTALL file maybe?

@dutkalex
Copy link
Contributor Author

dutkalex commented Feb 7, 2024

As I can't make a clean PR for the wiki, here is the documentation for the CMake installation.
Installation-CMake.md

I would also replace the first paragraph in the Installation.md file with something like:

Here, we discuss how to install t8code from the github repository on a linux machine using 
the autotools build system. Alternatively, t8code can also be installed using CMake (then 
see [this page](https://github.com/holke/t8code/wiki/Installation-CMake) instead).

@holke @jmark Anything more you need to merge this?

@dutkalex dutkalex requested a review from jmark February 7, 2024 14:28
dutkalex and others added 2 commits February 16, 2024 17:38
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
@dutkalex
Copy link
Contributor Author

dutkalex commented Feb 16, 2024

There are still some issues to solve and some things to do. For example, what is missing is a proper test of the cmake build system via Github CI where also all tests are passing.

This looks to me like a "chicken and the egg" situation: if we want to integrate into main, we need to have test coverage first, but in order for main to continue passing the tests, we need to integrate the code first. I think that since this is purely new stuff that can not break anything else that is already in production, having temporarily uncovered (but obviously flagged as experimental) code in main is a better alternative than to break the pipeline for everyone else...

As of now, the cmake build system is considered highly experimental and far from feature complete. I have to consult with the other core developers if we want to merge it with main nevertheless but with the clear note that this feature is experimental.

Migrating to a new build system takes time. Nevertheless, awesome work!

Oh ok! I did not understand that you plan on replacing the autotools build system with this one! I though of this work more as an alternative community-driven build system, as in p4est... This indeed increases the completeness and robustness requirements for this feature!

Yes, t8code is moving very fast and falling behind main is a common issue. So we have to keep up with it.

Yes and I think it is a good thing! I was pointing this out because the PR is already starting to get big (400+ LoC), and I am already pretty sure that new unit tests have been added since I did the porting! I think it would be better to merge this more or less as is, while being explicit about the fact that this in not yet production-ready, let people add their tests in both the autotools and cmake build systems, figure out together the edge cases as people start to use it, and improve it incrementally with additional PRs until we are confident enough to remove the experimental label...

@dutkalex dutkalex requested a review from jmark February 16, 2024 17:20
Copy link
Collaborator

@jmark jmark left a comment

Choose a reason for hiding this comment

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

@dutkalex Please consider these code changes. Other than that LGTM so far!

CMakeLists.txt Show resolved Hide resolved
example/CMakeLists.txt Show resolved Hide resolved
test/CMakeLists.txt Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
@jmark
Copy link
Collaborator

jmark commented Feb 20, 2024

This looks to me like a "chicken and the egg" situation: if we want to integrate into main, we need to have test coverage first, but in order for main to continue passing the tests, we need to integrate the code first. I think that since this is purely new stuff that can not break anything else that is already in production, having temporarily uncovered (but obviously flagged as experimental) code in main is a better alternative than to break the pipeline for everyone else...

Agree, the changes in this PR are non-intrusive.

Migrating to a new build system takes time. Nevertheless, awesome work!
Yes, t8code is moving very fast and falling behind main is a common issue. So we have to keep up with it.

Yes and I think it is a good thing! I was pointing this out because the PR is already starting to get big (400+ LoC), and I am already pretty sure that new unit tests have been added since I did the porting! I think it would be better to merge this more or less as is, while being explicit about the fact that this in not yet production-ready, let people add their tests in both the autotools and cmake build systems, figure out together the edge cases as people start to use it, and improve it incrementally with additional PRs until we are confident enough to remove the experimental label...

So, in accordance with the other core developers, here is the plan:

We leave the PR now as it is. A lot of stuff is already working and it is pretty much usable. We mark it as experimental and let it evolve slowly by incrementally adding new features one by one. At some point we can make the final switch and finally get rid of autotools for good.

Thanks @dutkalex for getting this project rolling!

dutkalex and others added 4 commits February 20, 2024 19:09
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
@dutkalex
Copy link
Contributor Author

Good news @jmark ! I have included verbatim your proposed changes as everything looked fine. Don't hesitate to tag me in future PRs about the build system, I'd be happy to continue helping if I can 🙃

@dutkalex dutkalex removed their assignment Feb 20, 2024
Copy link
Collaborator

@jmark jmark left a comment

Choose a reason for hiding this comment

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

@dutkalex One final edit to do. There were some file renamings just yesterday. Good timing! Sorry. But with this we can finally merge.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
dutkalex and others added 7 commits February 21, 2024 12:33
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
Co-authored-by: Johannes Markert <10619309+jmark@users.noreply.github.com>
@dutkalex
Copy link
Contributor Author

Done. @jmark

@jmark
Copy link
Collaborator

jmark commented Feb 21, 2024

Just for the record: #954

Copy link
Collaborator

@jmark jmark left a comment

Choose a reason for hiding this comment

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

Let's do it!

@jmark jmark merged commit 0405df3 into DLR-AMR:main Feb 21, 2024
8 checks passed
@dutkalex dutkalex mentioned this pull request Apr 22, 2024
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