Adapting to modern CMake#4
Conversation
|
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. |
| if(APPLE) | ||
| set_target_properties(bar PROPERTIES MACOSX_RPATH TRUE INSTALL_RPATH "@loader_path/") | ||
| else() | ||
| set_target_properties(bar PROPERTIES INSTALL_RPATH "\$ORIGIN/") |
There was a problem hiding this comment.
@jacobwilliams have you tried this without the escape?
|
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? |
|
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? |
|
Also, thanks for taking the time to do this! |
| ADD_CUSTOM_TARGET(distclean | ||
| COMMAND ${CMAKE_COMMAND} -P ${CMAKE_SOURCE_DIR}/distclean.cmake | ||
| ) | ||
| cmake_minimum_required(VERSION 3.15.2) |
There was a problem hiding this comment.
Is 3.15.2 actually the minimum version required? At the time of this writing I think that is pretty new, and perhaps if a slightly older version is supported by this template we should allow that.
This is from someone who works in an environment where often only older software versions are supported, so I know first hand the pain when you cannot use something shiny because your tools are too old.
There was a problem hiding this comment.
I only have 3.15.2 installed on the machine I developed and tested this with. It is possible that I could get an older version and try again to see if it builds. I suspect it may work back to 3.11 or so, but I believe in some cases I am using newer features or syntax that will limit the ability to backport and still call it "modern" CMake.
Let me try an older version and get back to you.
There was a problem hiding this comment.
target_link_options needs at least 3.13, so I don't think 3.11 will work (without changing the code).
|
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 |
|
The usage of Threads came about because of the previous usage of |
That's pretty great. It was a pretty big hole in the system before.
No problem. It might be useful to show 1-2 examples.
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. |
|
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. |
|
@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. |
|
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. |
|
A few more thoughts
|
|
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:
|
|
|
|
|
OK, so I see that elsewhere (https://mirkokiefer.com/cmake-by-example-f95eb47d45b1) folks are advocating for the 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. |
|
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). |
|
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. |
|
@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. |
|
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! |
|
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. |
Closes #3
Closes #1
This is my first attempt to overhaul this project, refactoring into a modern CMake approach. Comments/feedback are welcomed.