LLNL / libROM

Data-driven model reduction library with an emphasis on large scale parallelism and linear subspace methods
https://www.librom.net
Other
204 stars 36 forks source link

[feature/replace-autotools-with-cmake] Replace the creaky GNU Autotools build system with CMake #20

Closed goxberry closed 4 years ago

goxberry commented 5 years ago

Given some of the issues encountered with the GNU Autotools build system (e.g., comments in #6, #12), among others, this PR is intended to be a request for comment on using CMake instead.

This PR does:

This PR does NOT:

goxberry commented 5 years ago

cc: @slmcbane, @chldkdtn

goxberry commented 5 years ago

This PR should be complete enough to test on the LC or on a MacBook Pro managing package installations via Homebrew and/or Spack. This PR should also work on MacBook Pros using MacPorts to manage package installations, but I don't use MacPorts, and thus have not tested this PR on such a configuration.

goxberry commented 5 years ago

Also, in the event that #12 is merged, a FindScaLAPACK.cmake would probably need to be written. Copying the contents of a similar version from a BSD-licensed source is likely to suffice, or to yield an 80% solution requiring minor modifications.

rw-anderson commented 5 years ago

Adding cmake is a nice feature. Don't remove autotools.

goxberry commented 5 years ago

@rw-anderson I’ll see what I can do to accommodate your request without introducing too much duplication. One conflict I will have to resolve is how the Fortran name-mangling macros are generated. Another conflict would be hacking #6 to restore correct RPATH linking.

As a maintenance matter, one build system is preferable to two. On at least a couple projects I’ve worked on, keeping both build systems in sync becomes an additional technical burden that people are generally unwilling to bear, and so one system becomes the “preferred” system that is more often kept up-to-date than the other build system. Building the software on each system then becomes another required regression test. I think this technical approach isn’t cost-effective long-term, but I am willing to accommodate it if the project is willing to incur the additional maintenance costs that arise.

slmcbane commented 5 years ago

Also, in the event that #12 is merged, a FindScaLAPACK.cmake would probably need to be written. Copying the contents of a similar version from a BSD-licensed source is likely to suffice, or to yield an 80% solution requiring minor modifications.

I have something that works on my system and on LC systems for my C++ wrapper that's a WIP. I'm not sure it stands up to CMake standards since it's my first experience using it.

goxberry commented 5 years ago

I have something that works on my system and on LC systems for my C++ wrapper that's a WIP. I'm not sure it stands up to CMake standards since it's my first experience using it.

@slmcbane if you're writing a package, the most helpful thing you can do is write a CMake "config" file that other projects will use to detect the settings used to build your wrapper.

Aside from that, the build system can be cleaned up. Of the build systems I've worked with (CMake, GNU Make, SCons, GNU Autotools, Apache Ant), CMake seems to have more "style guides" that are readable than the alternatives. There are a slew of Autotools resources, but most of them seem like they require prohibitive investments of time to understand, like one of the GNU tutorials that has a several hundred slide presentation. For CMake style, check out Effective Modern CMake, CGold, Using Modern CMake Patterns to Enforce a Good Modular Design, and Effective CMake.

In general, across all build systems I've worked with, it's difficult to find examples of good style because there's much more focus on the code being compiled than on the build system. So don't worry about starting out with good style, just focus on writing something to put it out there, and then make it incrementally better -- this process is the best way to get better at build systems (and other skills that involve creative output; Ira Glass has a good, widely shared quote on this process).

slmcbane commented 5 years ago

Do you have a good example of a CMake "config" file that I can look at to see what it should do? I do want to integrate my code in libROM as soon as I have tests for all of the functionality used, it will make expanding on anything related to the static SVD much easier.

goxberry commented 5 years ago

@slmcbane This KitWare wiki page seems like a good start.

goxberry commented 5 years ago

Still to do:

goxberry commented 5 years ago

24 enables sidestepping the Fortran name mangling macro issue by eliminating the need for such macros entirely.

goxberry commented 5 years ago

Now I can make the configuration headers output by each build system essentially the same as those generated by #6, so all that needs to be done is to put documentation in the same place as the build system in #6, and the two build systems can coexist and "do the same thing".

That said, I'm more likely to make the CMake build system more feature-rich, because it's easier to do things like automatically download dependencies, or enable compiler wrappers, linters, coverage analysis, and so forth in CMake. Each build system should be able to build the software, but it's already easier to do so with CMake on my LLNL MacBook Pro than it is with #6 on my LLNL MacBook Pro. I am hopeful that the fixes to the Autotools macros I submitted upstream will help with building this software on a MacBook Pro.

goxberry commented 4 years ago

@chldkdtn Here's a status update:

I'm still working on resolving the static SVD bug. Once that's done, I can merge this PR and install ScaLAPACK on the LC.

goxberry commented 4 years ago

The MPI bug arises due to scalapack_f_wrapper.f90:317, based on stepping through the code with LLDB.

slmcbane commented 4 years ago

Found the problem; the destructor for StaticSVD calls release_context from the ScaLAPACK wrapper, which in turn calls blacs_gridexit to free resources used by a BLACS context. This in turn must call MPI but is called after MPI_Finalize.

This might be fixed in the test by wrapping the basis generator in a smart pointer and explicitly free-ing it before MPI_Finalize but there must be a more elegant way.

goxberry commented 4 years ago

I just added a line to random_test.C to call the destructor explicitly before MPI_Finalize().

goxberry commented 4 years ago

(i.e., https://github.com/LLNL/libROM/pull/20/commits/cdf208d2219cd71827e2102027411c086f636e4b#diff-23a7c1e92bc7795dab41e790483eab2eR265)

goxberry commented 4 years ago

Change of plans: I'm going to try to get this version of libROM to work with MKL so I don't have to install anything new, and so you guys can use the Intel MKL version(s) on the LC. I managed to find an MKL module that did most of the work and required minor fixes, and I've tested it on my laptop. It seems to work on my laptop, so we'll see how it fares on the LC.

goxberry commented 4 years ago

I get a memory leak with MKL in random_test.C, and still need to diagnose/fix that; I just want to get something that builds and mostly works so this PR isn't a blocker.

goxberry commented 4 years ago

@chldkdtn You should be able to compile libROM with the Intel 18 (or 19) compilers on the LC by running scripts/configure_ic18-toss_3_x86_64_ib-librom-dev.sh.

goxberry commented 4 years ago

I've tested this PR successfully on the following configurations without any runtime errors:

Given that this PR is a blocker for @chldkdtn (and I think @dylan-copeland, maybe?), I'm going to merge it now that it works. Feel free to ping me with follow ups re: issues encountered.

My next priority is going to be dealing with the PR backlog, likely starting this weekend with @jtlau's PR.