Switched build system to use CMake instead of Make

[TedStudley]

The directory structure has been changed slightly to support this (no more submodules in the source directory) but most things should remain the same.

We should now support both Doxygen and Google Test.

fixes #7, #8, and #13.

Tests don't work yet.

[icherkashin]

The directory structure has been changed slightly to support this (no more submodules in the source directory)

The subdirectories in the "source" directory could be kept. The CMake commands that would make that possible are, for example,

   # Location of source files
   SET(SOURCE_DIR ${ROOT_DIR}/source)
   file(GLOB SOURCES ${SOURCE_DIR}/*.cpp ${SOURCE_DIR}/*/*.cpp)

Keeping the original directory hierarchy is desirable, from my point of view, since the purpose of each source file is less clear when all the source files are in the same directory. Perhaps, there are reasons for not keeping this structure?

[egpuckett]

Ivan brings up a good point. ASPECT has several levels of subdirectories in various directories. Part of my goal with this project is to create a look and feel similar (but not as complex!) as ASPECT. This makes it easier for me to move back and forth between projects and also helps students come up to speed moving from mc-mini to ASPECT. Ted is there a specific reason to no longer use subdirectories of source/ or src/?

[egpuckett]

I forgot another issue that may or may not be related to this PR. That is I would find it easier to have an advection routines directory, say, and a separate source file *.cpp for upwind, Lax-Wendroff (LW.cpp?), and Fromm. Fromm is the main focus of development and it really should have a different name, say 'BellMarcus' or BM' since it most closely resembles BellMarcus

[egpuckett]

I didn't mean to finish the last comment. I have a habit of changing source code file names more often than is probably appropriate in this context. However I would definitely like to separate out different algorithms into separate source files and I would like to change the name of the routine with in the name 'Fromm', since it is not simply Fromm's method. However it might be best to have a discussion concerning the new name or give me time to think about it.

[egpuckett]

Is the intent to continue to have mc-mini compile on Stampede? I think that would be good.

[TedStudley]

There were a couple of reasons why I removed the subdirectory structure.

1. The CMakeLists.txt is one which I took from another project that didn't have a recursive directory structure. I wanted to get the build system switched over with as little work as possible, and it would be more difficult to re-add support for recursive directories because: 2.The way that subdirectory globbing was done before, as Ivan proposed, isn't actually a proper recursive solution. It will only support a single level of subdirectory, and any below that will just be ignored without any indication. A proper solution would take a little extra work, and I'd rather have a slightly less featureful 'proper' solution instead of a solution which is broken in potentially unexpected ways.
2. I didn't really think anybody would have a serious issue with removing the subdirectories, since they really aren't needed for this project. 4 out of the 5 subdirectories have 2 or fewer files in them, and 2 of them have literally only one file with the exact same name as the containing file. I'll look into how recursive directories is done in Aspect and add a properly recursive solution.

It would be pretty easy to separate out different implementations into their own source files, which would also be a very good case for a properly recursive directory structure.

This should still work on Stampede, although I don't have an account there to be able to test it. CMake provides the 'find_package' directive which automatically finds the location of required libraries. This should work both on a local machine and on Stampede, without having to modify it. If it turns out that it doesn't actually work on Stampede, there's likely solution in a couple lines, although since I don't have access it would be difficult to find it, exactly.

[icherkashin]

2.The way that subdirectory globbing was done before, as Ivan proposed, isn't actually a proper recursive solution. It will only support a single level of subdirectory, and any below that will just be ignored without any indication. A proper solution would take a little extra work, and I'd rather have a slightly less featureful 'proper' solution instead of a solution which is broken in potentially unexpected ways.

You are correct: ASPECT uses FILE(GLOB_RECURSE TARGET_SRC "source/*.cc" "include/*.h") https://github.com/geodynamics/aspect/blob/master/CMakeLists.txt#L29

I think we should simply accept this solution and not modify the existing structure just for its own sake.

[TedStudley]

That would work, but could actually break testing. The new build system builds two targets: the executable and a stand-alone library which can be linked elsewhere. The google tests need the stand-alone library in order to compile separately from the main project. Globbing will automatically include 'main.cpp', which can't be compiled into the library.

I'll add a fix to re-add the directory structure in source.

[TedStudley]

TravisCI should be working now! The issue was that the version of Eigen3 which was available on Travis is older than the one that we use, so a couple important features were missing. I've gotten around this by having travis clone the up-to-date Eigen repository before compiling instead of using the outdated version.

[icherkashin]

The issue was that the version of Eigen3 which was available on Travis is older than the one that we use, so a couple important features were missing.

Are these important features used in the code? If so, what are they? It would be instructional to know.

Globbing will automatically include 'main.cpp', which can't be compiled into the library.

I understand your concerns. We could imitate Aspect and have the following structure (not entirely the same as in Aspect!):

source/main.cpp
source/mc-mini/other_subdirectories/source_files.cpp

You can, of course, simply list the source files manually in the cmake files, as you have done.

In my opinion, as long as the directory structure is preserved, either solution is good!

[TedStudley]

Most of the headers in the root Eigen include directory were missing. I didn't investigate everything that broke, mostly because it didn't seem worth the trouble to get it working with an outdated version of Eigen, but the sparse matrix module seemed to have a completely different interface in the earlier version.

I've worked on a couple of projects where all source files were automatically included via globbing, and I've noticed that there are a couple of issues that can tend to come up. most of them are more specific to scripting languages where require order is important, but I've come to think that trading functionality (even potential functionality) for convenience is generally unsustainable in the long run (and also a very un-C++ thing to do.)

Listing the source files manually is simple in a project of this size, although the 'real' solution along this particular path would be to have CMakeLists.txt files in each subdirectory, similar to what's been done with the project directories 'source' and 'tests'. As far as I can tell, there isn't a consensus on best practices for build systems for C++ projects like this.

[icherkashin]

Most of the headers in the root Eigen include directory were missing. I didn't investigate everything that broke, mostly because it didn't seem worth the trouble to get it working with an outdated version of Eigen, but the sparse matrix module seemed to have a completely different interface in the earlier version.

Thank you! It could be useful to know the version of Eigen that failed to work, so we could know which minimum version of Eigen is required for the code.

I've worked on a couple of projects where all source files were automatically included via globbing, and I've noticed that there are a couple of issues that can tend to come up. most of them are more specific to scripting languages where require order is important, but I've come to think that trading functionality (even potential functionality) for convenience is generally unsustainable in the long run (and also a very un-C++ thing to do.)

The issue isn't so dramatic: again, Aspect uses "globbing" and doesn't suffer from it. But, certainly, there are implications of this choice, although they are rather intricate and, I suspect, will never come up with this project!

As far as I can tell, there isn't a consensus on best practices for build systems for C++ projects like this.

This is true. There are examples of both approaches being used successfully.

[icherkashin]

Let's merge this pull request with the main branch since:

1. The make build system has been successfully replaced with cmake.
2. The directory structure in source remained the same, which is desirable at this stage of the project.

We should address other issues after merging this with master, since they simply concern refining the cmake files.

[hlokavarapu]

Although in the root directory, findPackage(Eigen3) is specified in the CMakeLists.txt, CMake by default doesn't have an Eigen3 module file. Can we include the FindEigne3.cmake file as part of the code?

[hlokavarapu]

Also, the build fails when the DEBUG flag is toggled. The culprit is the display function in dataWindow.h and the problem is the signature of the that function. Here is the fix:

const std::string displayMatrix() 
  {
std::cout << Eigen::Map<Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor> >(_basePtr, _nYCells, _nXCells).colwise().reverse();

return ""; }

[TedStudley]

I should have included FindEigen3 in the cmake directory in the root of the project. Does CMake not find that when trying to run on your machine?

displayMatrix should really be changed to return a string representation of the matrix rather than directly outputting it. The main reason this is a problem is that we should be able to pass what we want to be displayed to whatever output handler that we want, especially (once it's in the project) a logger. I'll fix the function for now, but plan on revisiting it to address this (and switch from using the DEBUG flag to using logging levels.)

[icherkashin]

Although in the root directory, findPackage(Eigen3) is specified in the CMakeLists.txt, CMake by default doesn't have an Eigen3 module file. Can we include the FindEigne3.cmake file as part of the code?

I should have included FindEigen3 in the cmake directory in the root of the project. Does CMake not find that when trying to run on your machine?

Yes, FindEigne3.cmake was in cmake since this commit: https://github.com/TedStudley/mc-mini/commit/d32ab8106cd2d77c747aaad480c9d9380abc32af

displayMatrix should really be changed to return a string representation of the matrix rather than directly outputting it. The main reason this is a problem is that we should be able to pass what we want to be displayed to whatever output handler that we want, especially (once it's in the project) a logger. I'll fix the function for now, but plan on revisiting it to address this (and switch from using the DEBUG flag to using logging levels.)

The problem is due to an inadvertent change I've made in my _mc-miniimprovements pull request: I changed the return type to void. We should have a comment that would warn about the "in-progress" status of this function.

[TedStudley]

That function has always been mostly a 'convenience' function, just to be able to debug the shape and overall contents of the matrices. I had actually noticed the change in the pull request, but assumed that the stream operator would be overloaded in a way that ostream& ostream::operator<<(void); would do nothing and return the same stream. The only reason it returns anything to begin with is so that it can be inlined nicely into a logging statement.

I think that the correct behavior of this function, in order to maintain operability with the rest of the code and also be usable in non-stdout logging statements, is to just return a string representation of the matrix. The current implementation is just something that happens to have behavior that enables us to use it as though it did that in all of the places where it's used.

I'm going to see if I can update the travis configuration to also compile with/without the DEBUG flag set, so that we'll be able to catch these sorts of bugs more easily, as well, and we should add a comment to the effect of the proper usage/behavior of the displayMatrix function.

[hlokavarapu]

So Ted, the Travis CI system, are you running the process on your local home machine?

[TedStudley]

No, Travis is a (free) third-party service. Not sure how they stay profitable, but I'm not complaining, either. You can see the status of the build here.

I'd like to get an actual CI system like Jenkins working, in order to do automated testing instead of just an automated build, but we would need a dedicated machine in order to do that... I believe that Gerry has a machine that we can use, although I'll need to make sure.

[egpuckett]

Hi All,

Excellent work on all of your parts. I spoke with Bill Broadley this afternoon and he suggested we simply implement mc-mini build testing with Jenkins on the virtual machines he maintains for CIG and then have Jenkins call TravisCI as needed. I do not have a grasp of all of the details of Jenkins vs TravisCI, etc., but this sounds like the best approach to me. Here's how it benefits CIG: The new staff scientist hire Tyler at CIG is currently implementing Jenkins on the CIG virtual machines for Calypso. The ultimate goal is to have him implement Jenkins testing (and whatever else) for ASPECT. However, apparently they are looking for a smaller project for Tyler to implement before he tackles ASPECT. Thus, Bill suggested we have Tyler implement Jenkins plus TravisCI for mc-mini first, when he is finished with Calypso.

Bill was planning to bring this proposal up in the CIG staff meeting this afternoon. I have not talked with home since, so I need to see if Louise and others agreed with Bill's suggestion.

Any comments?

Ted: Is waiting for Tyler to finish his Calypso work going to be an issue? If so, we can still use my 'old' machine until Tyler takes over, assuming Bill's suggestion was approved.

• G

[TedStudley]

A virtual machine would be perfect for build testing and automating benchmark runs! It shouldn't be difficult to get support for using a VM to automate testing/building, since any skills gained implementing that can very easily be transferred to many of CIG's other projects.

Jenkins and TravisCI are largely the same type of tool, although with extremely different scopes to their usage. Both tools provide features to enable Continuous Integration, which is the practice of merging commits from all team-members into the master branch on a regular basis. This is typically accomplished by automated testing, both ensuring that the codebase compiles and that all of the tested code hasn't been broken, as well as ensuring any other standards that you might want to impose on the codebase.

TravisCI focuses mostly on ensuring that the codebase still compiles after every commit/pull request. It could be modified to run a test suite as well, but isn't inherently designed to support that, making it kinda hacky.

Jenkins is much more powerful and configurable than TravisCI, and can be used to automate running testing suites, checking general code quality metrics (using tools like cppcheck or vera++), pre-compiling binaries for deployment, and pretty much any other task that you can think of.

I think it would be a great idea for Tyler to try implementing Jenkins for mc-mini, since that should be similar to (but a lot easier than) implementing it for ASPECT. I don't think that waiting for him to be available will be an issue, since having Jenkins available isn't really necessary, just extremely useful. Seeing as we don't really even have any tests yet, there's really no rush to get Jenkins working.

@Vandemar, have you managed to get mc-mini to build with the new build system? If so, I'd like to get the pull request merged into master, since it's currently blocking a number of other subtasks related to testing and documentation. Otherwise, I need to figure out what's wrong with it and fix that. Since the merge request touches so many files, I'd like to get it merged in ASAP to minimize merge conflicts.