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

How to make the build process more robust (and more portable) #15

Closed
sloede opened this issue Oct 22, 2020 · 1 comment · Fixed by #24
Closed

How to make the build process more robust (and more portable) #15

sloede opened this issue Oct 22, 2020 · 1 comment · Fixed by #24
Labels
question Further information is requested

Comments

@sloede
Copy link
Member

sloede commented Oct 22, 2020

This issue is mainly motivated by the MacOS-specific fixes in #14, but also by the fact that we currently use 4 different environment variables to control the build process for this package (6 variables if you count convenience variables as well).

So far, the installation process for P4est.jl can be described roughly as follows:

  1. We first find out where the p4est library and header files are located.
  • If this is a p4est-build with MPI support, we also figure out where the MPI header files are located.
  1. We then generate C bindings for p4est using CBindingGen.jl and save them to a local file
  2. In the P4est.jl package itself, we include the file with the bindings and add additional usings etc.

This procedure follows the recommendations by CBindingGen.jl, which states in its README.md:

This package should only be used at package build time when you wish to generate bindings to a particular C library on the system or one built and installed at build time. The generated bindings file can then be included from your package code. Bindings files created by this package should not be committed with your package and they are not meant for editing by lowly humans once generated.

However, there at least two issues with this approach:

  • It requires users to have all header files: Since we need the header files for the automatic generation of the C bindings, they must be available on the user's machine. While this is a reasonable expectation in general for p4est and MPI, it might not be the case if users install them via their package managers. Also, as we can see in Fix build step on MacOS (workaround) #14, it requires to jump through hoops at least on MacOS (and I can't even tell what's going to happen on Windows yet).
  • It requires users to know where the header files are located: This might be again a reasonable expectation on HPC systems, but it's a completely different story for libraries installed via package managers (e.g., on Ubuntu even MPICH and OpenMPI do not have similar include paths).

Both issues could be avoided by using a pre-generated C binding file (or two, for p4est with and without MPI enabled). However, this would probably create new issues, such as dependencies on the versions of p4est and CBinding.jl. OTOH, in this case using an external p4est library would only require to specify the path to the p4est shared library file.

A third option could be to combine the current approach with a pre-generated C binding file. In this case I would probably use the pre-generated C bindings file as the default, but allow the user to specify include paths for p4est and MPI and then re-generate the C bindings. The downside of this approach is that it requires an even more complicated build process (that becomes harder and harder to test in the CI), but it would (might?) provide maximum convenience for "standard" users together with full flexibility for advanced users.

To me, this issue blocks #4, so I'd like to solicit your input on this. Personally, I am leaning towards the third option.

@sloede sloede added the question Further information is requested label Oct 22, 2020
@ranocha
Copy link
Member

ranocha commented Oct 23, 2020

The third options seems to be the best one for users - I prefer that one, too 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants