mreineck / ducc

Fork of https://gitlab.mpcdf.mpg.de/mtr/ducc to simplify external contributions
GNU General Public License v2.0
13 stars 12 forks source link

cmake build system #37

Closed DiamonDinoia closed 1 month ago

DiamonDinoia commented 2 months ago

Hi Martin,

I drafted a small CMakeLists.txt to build the project let me know what you think!

Thanks, Marco

mreineck commented 2 months ago

Thank you very much! I will have a look tomorrow!

mreineck commented 1 month ago

Hi Marco,

I did a few tests and almost everything seems to run fine! One thing I didn't manage to do is to make the DUCC_ARCH_FLAGS appear in the compiler calls; does this require an option in the cmake call?

More of a philosophical thing: I'm slightly worried that the presence of this CMakeLists.txt might inspire someone to make a ducc0 C++ package for various Linux distributions, and in my eyes this would be a bad idea ... the C++ interface should not be considered stable, and no one wants a shared library in /usr/lib that breaks its interface every few months.

In my opinion, ducc0 should be either used as a Python package (where I guarantee fairly stable interfaces), or as a submodule in another C++ package, where the code is linked in statically, to minimize the risk of breakages. Do you think that adding cmake support might send the wrong message?

Thanks, Martin

DiamonDinoia commented 1 month ago

Hi Martin,

I made some improvements. Release build is not the default and on windows I emulate a -march=native (I have not tested this as I do not have any windows machine...)

To have DUCC_ARCH_FLAGS appearing in your build you should set the cmake .. -DCMAKE_BUILD_TYPE=Release The user can manually override the DUCC_ARCH_FLAGS at command line in the same way.

We could add a github workflow that builds using cmake to test that nothing is broken.

I cannot be sure that some distro might distribute a ducc cpp package but I do not think having multiple build options will influence this. We could build the python wheels from cmake directly and disable the installation of the static library by default. We could also have a warning on the readme that states that the cpp interface is not stable yet and to ask the developers before distributing a non python interface.

I think c++ developers mostly pull dependencies with submodules/fetch_content and do not rely on the package manages so having a cmake file can indeed boost adoption.

Is it a lot of work to have a small set of exposed header that are stable, do you plan on building that in the future?

Thanks, Marco

mreineck commented 1 month ago

Thank you for the changes! I'm happy with just having a warning in the README that the C++ interface is unstable.

I think c++ developers mostly pull dependencies with submodules/fetch_content and do not rely on the package manages so having a cmake file can indeed boost adoption.

This is the way I hope things will go: pull in a specific version into your own software, and update cautiously from time to time, but not necessarily for every ducc release.

Is it a lot of work to have a small set of exposed header that are stable, do you plan on building that in the future?

I would rather want to avoid it, since I'm thinking of several future changes that will affect the interface:

So, unless it is really urgently required at the moment, I'd rather not nail down anything :-)

DiamonDinoia commented 1 month ago

That sounds good. I think a warning would suffice.

mreineck commented 1 month ago

I'll merge now and will update the ChangeLog etc. subsequently. Thanks again!