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

Continue CMake build system #103

Open
cburstedde opened this issue Mar 12, 2021 · 14 comments
Open

Continue CMake build system #103

cburstedde opened this issue Mar 12, 2021 · 14 comments
Labels
enhancement upforgrabs Desired functionality, currently unassigned

Comments

@cburstedde
Copy link
Owner

cburstedde commented Mar 12, 2021

Description

The CMake build system is supported on branch develop, but not yet complete.

Proposed solution

  • The archive created by cpack is lacking some .c source files as well as example and test.
  • The archive created by cpack shall include all files required to run ./bootstrap && configure && make -j check V=0 in the unpacked tree.
  • Generally, the cpack archive should be as similar as possible to the make dist tarball by automake.
@cburstedde cburstedde added enhancement upforgrabs Desired functionality, currently unassigned labels Mar 12, 2021
@cburstedde cburstedde added this to Conception in Incremental improvements via automation Mar 12, 2021
@cburstedde
Copy link
Owner Author

Additionally:

  • Add a new CI job that runs cpack, unpacks the archive und then runs the autotools chain.
  • Add a new CI job that calls make dist, unpacks and then builds, tests and packs using cmake.

@cburstedde
Copy link
Owner Author

cburstedde commented Mar 23, 2021

A couple more items to align the CMake to the automake build:

  • Currently, make install without explicit prefix installs into sc/local. Change to local.
  • When installing and sc is a submodule, the install of sc should go into the p4est install prefix, not under sc.
  • The CMake build creates libp4est.a, libp6est.a and libp8est.a. To align with autotools, it should be all in libp4est.a.

@cburstedde
Copy link
Owner Author

cburstedde commented Apr 28, 2021

  • make install creates uppercase SC and P4EST files/directories. Preferred convention would be lowercase.

@scivision
Copy link
Contributor

I think to make "examples" be a full test of the "installed" p4est from a single project instead of two separate projects as I have now would require making CMake be a "superbuild" where a new top-level CMakeLists.txt contains two ExternalProjects--one for p4est, one for p4est examples.

The reason for examples as a separate project or subproject is to test that CMake packaging is correct--it is a common pitfall to have problems with linking, compiling or finding the installed library due to something wrong with p4est CMake package. These problems only show up when treating p4est as a separate package upon a separate project or subproject, but are how virtually all non-p4est-dev users will use p4est.

@scivision
Copy link
Contributor

scivision commented Apr 29, 2021

For the lowercase SC/P4EST install subdirectory naming, yes I think that can be done with a few extra lines of code

To make one libp4est.{a,so} regardless of whether or not p8est,p6est are also built can be done a couple different ways. To do it most like a typical Makefile:

add_library(p8est OBJECT ...)
add_library(p6est OBJECT ...)
...
target_link_libraries(p4est PRIVATE p6est p8est)

this creates object files for building into p4est, like a traditional Makefile.
For legacy compatibility reasons, target_link_libraries is overloaded in cases like this to mean "transitively include this object file and its associated CMake properties" instead of strictly "linking"

There might be a couple additional touchups as I might assume in some other places (like for install/packaging) that there are up to 3 library files instead of only 1.

@scivision
Copy link
Contributor

To specifically control tests MPI rank, lines like

${MPIEXEC_EXECUTABLE} ${MPIEXEC_NUMPROC_FLAG} ${Ncpu} $<TARGET_FILE: ...

become like (assuming fixed rank 2 is desired)

${MPIEXEC_EXECUTABLE} ${MPIEXEC_NUMPROC_FLAG} 2 $<TARGET_FILE: ...

@cburstedde
Copy link
Owner Author

I think to make "examples" be a full test of the "installed" p4est from a single project instead of two separate projects as I have now would require making CMake be a "superbuild" where a new top-level CMakeLists.txt contains two ExternalProjects--one for p4est, one for p4est examples.

The reason for examples as a separate project or subproject is to test that CMake packaging is correct--it is a common pitfall to have problems with linking, compiling or finding the installed library due to something wrong with p4est CMake package. These problems only show up when treating p4est as a separate package upon a separate project or subproject, but are how virtually all non-p4est-dev users will use p4est.

Sounds all very good -- removed change of examples from issue.

@cburstedde
Copy link
Owner Author

* The tests on make check run with 2 MPI ranks. Make ctest behave the same if not so.

This is done.

@PatrickThomasma
Copy link

I am using the p4est develop branch right now and I was wondering if there was a way to build the source files inside of p4est/examples ? The CMakeLists inside there has problems building it and im not sure if there's something I need to add in order to get them to build.

@cburstedde
Copy link
Owner Author

The develop branch is current again. If you think the examples are not built, please feel free to have them built by default, the same way it is done with the autoconf build.

@cburstedde
Copy link
Owner Author

I've updated the p4est tests such that they survive if P4EST_HAVE_ZLIB or SC_HAVE_ZLIB are not defined.

The new downside is, however, that a missing ZLIB define now goes unnoticed. Last I've seen, there are several CMake builds in the CI that appear to not set these defines. This would be nice to have fixed so the defines are always consistent.

@cburstedde
Copy link
Owner Author

* Add a new CI job that runs cpack, unpacks the archive und then runs the autotools chain.
* Add a new CI job that calls make dist, unpacks and then builds, tests and packs using cmake.

This would be the ultimate test; cc @dutkalex for reference.

@cburstedde
Copy link
Owner Author

cburstedde commented Feb 15, 2024

I am using the p4est develop branch right now and I was wondering if there was a way to build the source files inside of p4est/examples ? The CMakeLists inside there has problems building it and im not sure if there's something I need to add in order to get them to build.

The examples in both sc and p4est should be built by default on make. Only the tests are built separately and run by make check. If this is not so with the current CMake builds, please feel free to fix.

@cburstedde
Copy link
Owner Author

Just noting some differences before I forget: the test programs shall be named p4est_test_partition, p8est_test_partition, etc., and cmake links static by default while autotools link dynamic by default. What to do about it we shall consider another day.

The file names should be aligned to those generated by automake. About static/dynamic builds, I'm fine using the CMake defaults and only override when specified explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement upforgrabs Desired functionality, currently unassigned
Projects
Development

No branches or pull requests

3 participants