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

Proof-of-concept: Integrate subset of p4est source files #1043

Closed
wants to merge 34 commits into from

Conversation

jmark
Copy link
Collaborator

@jmark jmark commented Apr 23, 2024

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.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)

@jmark jmark added enhancement Enhances already existing code cleanup Cleans the code dependencies Pull requests that update a dependency file labels Apr 23, 2024
@jmark jmark marked this pull request as ready for review April 23, 2024 09:54
@cburstedde
Copy link
Collaborator

cburstedde commented Apr 23, 2024

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.

@cburstedde
Copy link
Collaborator

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?

@jmark jmark changed the title Enhancement: Integrate subset of p4est source files Case study: Integrate subset of p4est source files Apr 23, 2024
@jmark jmark changed the title Case study: Integrate subset of p4est source files Proof-of-concept: Integrate subset of p4est source files Apr 23, 2024
@jmark jmark marked this pull request as draft April 23, 2024 11:30
@jmark
Copy link
Collaborator Author

jmark commented Apr 23, 2024

@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.

@jmark jmark added the draft Enhance the visibility that this is a draft. label Apr 23, 2024
@cburstedde
Copy link
Collaborator

@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.

@holke
Copy link
Collaborator

holke commented Jul 15, 2024

we decided not to continue this route

@holke holke closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleans the code dependencies Pull requests that update a dependency file discussion draft Enhance the visibility that this is a draft. enhancement Enhances already existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants