cmake_fortran_template icon indicating copy to clipboard operation
cmake_fortran_template copied to clipboard

Adapting to modern CMake

Open emanspeaks opened this issue 5 years ago • 21 comments

Closes #3 Closes #1

This is my first attempt to overhaul this project, refactoring into a modern CMake approach. Comments/feedback are welcomed.

emanspeaks avatar Aug 23 '19 03:08 emanspeaks

I'll add that I removed some of the code specific to addressing "bugs". I think many of those bugs have since been fixed, and would request that any other bugs found from this point onward should be addressed by new issues against this "vanilla" template. If any of the original bug workarounds need to be restored, though, please let me know.

emanspeaks avatar Aug 23 '19 03:08 emanspeaks

I notice that all of the handling for MPI and OpenMP have been removed, and seem to be replaced with Threads. Is there a reason?

SethMMorton avatar Aug 23 '19 04:08 SethMMorton

I also notice that the section on Fortran compile flags has been removed. You mentioned that things are different in modern CMake, but it's hard for me to see where/how they get set. Can you give a short explanation?

SethMMorton avatar Aug 23 '19 04:08 SethMMorton

Also, thanks for taking the time to do this!

SethMMorton avatar Aug 23 '19 04:08 SethMMorton

OpenMP and MPI are now natively supported for Fortran by CMake, so the code in the original template is no longer necessary. There is also now a standard CMake module for testing valid compiler flags just as there are for C and C++. I considered both of these things well documented in CMake documentation and/or easily "Google-able."

As for setting flags, these are now addressed by target_compile_options() and target_link_options(), which are used in both examples in src/. I did mean to come back and add some examples using the original flags from SetFortranFlags, but clearly forgot about that. I can also add comments highlighting these functions when they are used.

emanspeaks avatar Aug 23 '19 05:08 emanspeaks

The usage of Threads came about because of the previous usage of ${CMAKE_THREAD_LIBS_INIT} in the example usage of LAPACK flags. This flag doesn't seem necessarily required for use with LAPACK, but either way the value is populated by the Threads package by CMake.

emanspeaks avatar Aug 23 '19 05:08 emanspeaks

OpenMP and MPI are now natively supported for Fortran by CMake, so the code in the original template is no longer necessary.

That's pretty great. It was a pretty big hole in the system before.

I did mean to come back and add some examples using the original flags from SetFortranFlags, but clearly forgot about that.

No problem. It might be useful to show 1-2 examples.

The usage of Threads came about because of the previous usage of ${CMAKE_THREAD_LIBS_INIT} in the example usage of LAPACK flags. This flag doesn't seem necessarily required for use with LAPACK, but either way the value is populated by the Threads package by CMake.

At least when I was still using Fortran, I found that yes in general threads was not needed for LAPACK unless you were using MKL, in which case it needed threads to do the multi-core operations.

SethMMorton avatar Aug 23 '19 14:08 SethMMorton

I think that in a few days I will try to make a commit to master that adds a changelog or something. That way when this gets merged there will be a record of what is different.

SethMMorton avatar Aug 23 '19 14:08 SethMMorton

@emanspeaks This is really starting to look good! I will make that commit in the near future, I promise... I have a newborn at home so getting the time to do "fun" things is harder than it was in the not-so-distant past.

SethMMorton avatar Aug 28 '19 05:08 SethMMorton

I have pushed a change where I added a CHANGELOG and also referenced it in the README. I think it would be nice if this update specified what changed in the CHANGELOG so users already using the template can know what is different.

SethMMorton avatar Aug 31 '19 22:08 SethMMorton

A few more thoughts

  • I notice now that distclean.cmake is gone. Is there equivalent functionality from cmake itself?
  • The code to set up things like LAPACK, OpenMP, and MPI used to be in the top-level CMakeLists.txt file, but now it is in the CMakeLists.txt for the foo subcommand. It seems to me that this would only set up those libraries for the foo subcommand (in this case the executable) but not for bar (in this case the library). Would not users want to use those libraries want it for all subcommands of the project?
  • Similarly as above, the compile flags only seem to be set for foo.
  • I used the template and ran make, and found I had a very hard time finding the resulting executable and libraries. The original template had some code to put it into a bin and lib directory in the build root - is there a reason this was removed?

SethMMorton avatar Aug 31 '19 23:08 SethMMorton

Cool, thanks, I'll try to get back to work on this later this week and incorporate comments/update changelog, etc. And no worries re: new dad schedule :-)

Trying to address your notes above as I can:

  • The beauty of (modern) CMake is that if you ensure your make process only modifies files in the build directory, simply deleting the build folder should have the same effect as what distclean did from what I could tell by reading the script. For stuff like this that is just education/tips for future users on CMake best practices, that should be addressed in documentation. Thus far I have made very limited contributions on the documentation front...I'll try to add more later to round out this and some other related changes that are more about best practice and actual usage than CMake template code proper.

  • In modern CMake, you are never supposed to change flags at the global level unless you have a good reason to...and most of the purists would say you never should have a good reason to do so. Everything should be applied on a per-target basis. In the example, I only applied the flags to foo to play the game of pretending they only apply to foo and are not necessary for bar (if foo uses OpenMP but bar does not, there is no reason for bar to use those flags). Since bar is a library, however, it's probably more likely in practice that bar would have special flags that, if you use the library in another target, need to also be applied to that target (if foo uses bar and bar uses OpenMP, as an example). If you apply the flags as PUBLIC or INTERFACE flags on the library, then those flags will also be used by any downstream target that uses the library. So maybe it would be better to move all the flag examples to bar instead. But I'm thinking that it would be better to have all the "common" flag examples in one place so people know where to look.

  • Placing things into bin/ and lib/ are now handled by the install CMake command. Following a build, you should do make install and it will create the bin/ and lib/ directories and populate them accordingly. Just make sure you set the install path as an argument to CMake if you want to override the default set in the CMake project file (and if you are the project developer, make sure you set an appropriate default!) This all has to do with the first bullet, which says that the entire build process should be completely self contained and not place files anywhere outside the build directory until/unless you explicitly tell it to "install" them.

emanspeaks avatar Sep 02 '19 01:09 emanspeaks

  • Make sense, if everything is in build the cleanup is simple. Looking at the content of that file, it seems like CMake used to put CMakeCahce.txt, cmake_install.cmake, install_manifest.txt, and CTestTestfile.cmake at the top-level of the directory. I don't know if that was because of how I had configured the template or if it is because it was how CMake used to behave - but now CMake only puts collateral into build and never into the top-level directory?
  • This also makes sense, but let me play devil's advocate. Suppose I wanted to have the same settings for all my subprojects. Would I have to copy/paste the same code in each CMakeLists.txt for each subproject, or is there a way to set up a function or macro to reuse the settings across projects?
  • While I agree that the install step of CMake will take care of putting the targets in the correct places, I am thinking about the development process, where one wants to test out their executables. I realize that the proper solution is to use CTest to run automated testing, in which case it probably does not matter where the executable lives, but in practice Fortran code is often written by academics that do not perform automated testing, and would rather manually test their code. In that case having easy-to-find build targets is a plus. Is there no way to get the targets (at least the executables) at the top-level of the build directory?

SethMMorton avatar Sep 02 '19 21:09 SethMMorton

  • Yes, you are correct that by default it does not put anything into top-level directory. You can change that behavior, but it is discouraged as part of the "build" step. If you want stuff in the top level, you should "install" it there.

  • The project that I have been developing that is driving this rewrite of the template is actually two projects: one that is an independent library, and a second one has an executable that uses that library. They each have their own CMakeLists.txt files in different directories. Because the I set special flags that are specific to the library in the library's CMakeLists.txt file, all I need to do is add the target_link_libraries for the executable and the appropriate additional flags are automatically applied. No additionally code necessary. Now, if I wanted to, say, globally enable some feature (let's say it's some obscure optimization flag we don't already have code for, for example) that applies to both things, then I could instead use add_compile_options, which sets flags for everything in the current directory and below (and just make sure the library is in a subdirectory of the current). But if the special flags arise simply from the use of the library, they should only be applied to the library. Consider "targets" in modern CMake like objects in object-oriented programming.

  • I would guess that there are really two major scenarios for the users you describe: either they develop in Linux and use the make command, or they develop in Windows and use Visual Studio. For users familiar with make, the idiom make; make install will be very familiar to them, so relying on install even for debugging/testing shouldn't be all that much additional overhead. For users of Visual Studio, there is no need to relocate the build products because even if they are buried in some obfuscated path, Visual Studio's debugger will know where to find them if we are debugging/testing. If this is the final release, then I think they should be expected to run the INSTALL target in VS to collect all the final versions of things to distribute. If there's another scenario I'm not picturing, I'm happy to address that. But modifying the path of build products during build should not be necessary and actually could break some build systems. My goal here is to try to set up the template to be proper CMake best practices, not try to protect for users who don't know how to use build systems.

emanspeaks avatar Sep 05 '19 15:09 emanspeaks

  • We should add protection that prevents users from doing an in-source build. I think in my original template I allowed that, which was why the distclean.cmake was needed.
  • Got it. The template or README should probably indicate somewhere how to do global vs. local configuration.
  • Maybe it's just me, but I have only ever used make install to actually install a program, not to ready it for testing. Every other project I have interacted with will put the final targets in a easy-to-find location within the build directory when just using make, then use make install to place into the system locations (/usr/local/bin or whatever). I see no problem with the configuration as-is for Visual Studio, but not having the build targets easily locate-able after running make did not pass the Principle of Least Astonishment for me. The template could have Make-only code to make this happen so that it does not interfere with users with IDEs. Is there a problem with having them copied (or even just soft-linked) to build/ (e.g. build/foo)? I want to make very clear that in no way am I advocating that we put these outside the build directory.

SethMMorton avatar Sep 05 '19 16:09 SethMMorton

OK, so I see that elsewhere (https://mirkokiefer.com/cmake-by-example-f95eb47d45b1) folks are advocating for the make; make install idom as you do, and they advocate to use -DCMAKE_INSTALL_PREFIX=blah when invoking cmake initially to set up the development "install" location.

This is very un-intuitive to me (since "install" implies the an installation outside my build area in my mind), but I have been out of the game for a while and I am not above accepting that things change.

Having said that, it is important to realize that this is a template, which means the target audience is folks who do not have enough CMake knowledge to write this code themselves (nor want to take the time to do so), so we should assume that they (like me) will not know to do this and be surprised/frustrated. We should make sure that how to do this is clear in the README.

If you want, we can table all of these documentation updates till after this PR is merged, and I can update the documentation after-the-fact in a separate PR.

SethMMorton avatar Sep 05 '19 23:09 SethMMorton

I think it's okay to wait on this PR until we have documentation updated accordingly, no rush to get it merged. I don't want to put something in master that people aren't going to understand!

Also, along the lines of your concerns about people finding the executables for debug: I have been using VS Code as my editor along with the CMake Tools extension. There's a way to have it automatically determine the path to the built executable when you try to launch it so that you don't need to go looking for it. I'm considering adding a VS Code template to go along with this template that leverages this and addresses your concerns about ease of use (assuming that the users are using VS Code, anyways).

emanspeaks avatar Sep 10 '19 16:09 emanspeaks

Are the config files you added VS code-specific, or can they work for other editors? If the template is going to comprehend editor configurations, it should probably do so for all the popular editors as well (like I tried to do for fortran compilers as well). Examples are sublime text, vim, emacs...

Otherwise I think that it is better to keep it limited to cmake, but I am willing to be convinced otherwise.

SethMMorton avatar Sep 12 '19 04:09 SethMMorton

@emanspeaks I am currently on paternity leave, so have time to get this done. Shoot me an email and let's work to get this released soon.

SethMMorton avatar Nov 13 '19 22:11 SethMMorton

Cool, sorry for the lack of recent progress. I was doing this in support of a project I was using at work that I've been pulled off to do something else. I had been meaning to circle back on this, but I'll have some time this weekend to follow up. Let's get this finished!

emanspeaks avatar Nov 14 '19 21:11 emanspeaks

Quick status update: I'd been recently running into some error messages in CMake that I can't quite explain. I lost a lot of time trying to debug those issues before I decided to scrap my repo and start over. I think something got corrupted at some point because CMake was still working fine on other projects. When I recloned the repo, now it seems to be working again here. I'll try to get back to working on cleanup as time allows now that I have something working again.

emanspeaks avatar Dec 06 '19 18:12 emanspeaks