DLR-AMR
Proof-of-concept: Integrate subset of p4est source files
Describe your changes here:
This proof-of-concept integrates a subset of p4est header and source files directly into the t8code source tree. Thus, pulling the whole p4est distribution as a git sub-module is not necessary anymore. This PR is motivated by several observations.
- t8code only uses a very small subset of p4est functionality.
- the used functionality wasn't changed for years and is stable.
- total time of t8code setup (cloning, bootstrapping, configuring, and compiling) is around 30% faster
- p4est unit tests are dropped. This should considerably speed-up GitHub CI and testing locally.
- reduces complexity and increases self-sufficiency, especially with regards to t8code's build system
- p4est's licensing model allows to do this. Of course, proper referencing and acknowledgement of p4est authors is incorporated.
- it was pretty straightforward to to (within an hour) without any big hassles or a lot of time investment
- there are no trade-offs or other disadvantages in terms of t8code development
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.scpto 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)
The immediate motives are understandable!
Reviewing what is possible now and will no longer be possible after merging:
- Writing code say to wrap a p4est connectivity in a t8code coarse mesh or vice versa without copying
- Reusing any t8code convenience functionality, say for mesh reading, in p4est, and vice versa
- Using some of the generic scatter/gather lnodes related node code from p4est in t8code
- Building a project against both p4est and t8code, since there will be duplicate files
This is a practically terminal split between the two codes that will be hard to reverse. It will force users to choose exlucisevly between the two libraries, while presently using t8code allows for using p4est as well with full flexibility. In other words, this PR implements a strategic decision of the highest relevance.
One way around the disadvantages might be to provide a build option to compile and link against an external p4est that is independently make installed, which would disable compiling the p4est files copied. This is a route that say a Debian maintainer may go. The caveat with this approach is: who will be continuously testing this build variant?
@cburstedde Thank you very much for your informative advice!
I relabeled this PR as proof-of-concept. As of now it is more of a case study to see what is generally feasible and to spark fruitful discussion.
@cburstedde Thank you very much for your informative advice!
I relabeled this PR as proof-of-concept. As of now it is more of a case study to see what is generally feasible and to spark fruitful discussion.
Thanks for your comment!
Copying hand-selected source files seems even less transparent than a fork, since the information gets lost what upstream commit they are copied from. Having a script to sync selected files from the latest p4est release may enhance reproducibility and ease maintenance.
I'm wondering however: Most of the t8code users rely on the p4est files from within the t8code sources. Eventually, there might be a divergence to the p4est upstream, which could cause the t8code/external p4est build to fail temporarily. If the t8code users insist on using the builtin version, will there be sufficient incentive to align to upstream p4est, or is this another tipping point where the two codes may go their separate ways (not mentioning the duplicate symbols issue)?
Another question is that of licensing. While it is fine to copy files from one GPL code to the other if GPL is all that is ever desired, a dual license can only be negotiated for t8code that is including some p4est files if the p4est copyright holders join the contract. It may be easier and more modular, for any customer, to go with a standard p4est license on the one hand and independently negotiate a pure t8code license on the other.
