Fortran fixes #108

zbeekman · 2017-10-29T20:26:38Z

Fixes BLT breaks simple Fortran w/ CMake 3.8 #105
- CXX must be enabled earlier if ENABLE_GTEST is set, otherwise a CMake error occurs
Fixes Why are you adding -ffree-form (GFortran) #106
- An option was added to allow users to specify FIXED, FREE or INFER for FORTRAN_FORMAT. Defaults to FREE: This is the old behavior for backwards compatibility. Setting FREE or FIXED cause CMAKE_Fortran_SOURCE to be set accordingly, setting INFER causes CMAKE_Fortran_SOURCE to NOT get set, so that the compiler handles detection of FREE vs FIXED source. Most compilers do this via the filename extension:
  - .f or .F: FIXED source formatting is assumed
  - .f90 or .F90: FREE source formatting is assumed
Allows user to not need to specify ENABLE_FORTRAN because this will default to TRUE IFF Fortran is detected already by CMake. (Query ENABLED_LANGUAGES global property.) Users will enable Fortran via project() command by default most of the time, and having to do this again for BLT is redundant and annoying, and unconventional/un-idiomatic.

zbeekman · 2017-10-29T21:48:33Z

Let me know if you want me to cleanup the commits so that my "bug fix" commits are in the commits that introduced the issue. I'm happy to perform an interactive rebase to squash the commits so that each one is an atomic set of self-consistent changes (which they are save for removing IN_LIST (eef788c) and fixing set_property() (8fda969).

zbeekman · 2017-10-29T21:50:16Z

I'm also thinking of stealing #99 and including the fix here and also setting up some dependent option logic (#107)

kennyweiss

Thanks for the contribution @zbeekman

kennyweiss · 2017-10-30T16:57:21Z

cmake/SetupCompilerOptions.cmake

+    elseif(FORTRAN_FORMAT MATCHES INFER)
+        message(STATUS "Fortran source formatting not set---the compiler may infer this from each source file")
+    else()
+        message(ERROR


I don't think ERROR is a valid keyword here.
We typically use FATAL_ERROR for this kind of message.
See: https://cmake.org/cmake/help/v3.8/command/message.html

You're totally right, my apologies.

kennyweiss · 2017-10-30T17:02:19Z

cmake/BLTOptions.cmake

@@ -53,8 +53,14 @@ option(ENABLE_COVERAGE   "Enables code coverage support" OFF)
 ################################
 # Build Options
 ################################
+get_property(_languages GLOBAL PROPERTY ENABLED_LANGUAGES)
+if(_languages MATCHES "Fortran")


I think ENABLED_LANGUAGES is a list -- https://cmake.org/cmake/help/v3.8/prop_gbl/ENABLED_LANGUAGES.html#prop_gbl:ENABLED_LANGUAGES

Does this only work if Fortran is the only enabled language?
What if multiple languages are enabled?

Since one of our core macros already uses if( ... IN_LIST ...) (see the blt_setup_target macro), I think it is safe to say that blt already requires cmake 3.3 or higher.

We should probably note this in our documentation, add a check in SetupBLT and set policy CMP0057 appropriately.

See: https://stackoverflow.com/questions/23323147/best-way-to-check-with-cmake-whether-list-containts-a-specific-entry for some other options for checking if an item is in a list.

Yes, ENABLED_LANGUAGES IS a list.

Fortunately MATCHES takes a regex so this check just looks for Fortran any where in the "list" (i.e. string with semicolons in it)

I noticed you were using IN_LIST, but I didn't see any calls to deal with CMP0057. I think it would be wise/safe to set CMP0057 to NEW.

I agree, IN_LIST is a much better solution.

To further elaborate, IN_LIST was causing CI failures because, AFAICT, CMP0057 was not set anywhere. My preference would be to set CMP0057 to NEW and switch back to IN_LIST (revert eef788c)

I created issue #109 about the required version of cmake and the CMP0057 issue.

kennyweiss · 2017-10-30T17:10:16Z

SetupBLT.cmake

@@ -82,6 +82,13 @@ if (NOT BLT_LOADED)
  endif()

  ################################
+  # Enable CXX required by GTEST
+  ################################
+  if(ENABLE_GTEST)


Not sure about this one.

I think we'd probably want the logic to go the other way.
I.e. rather than enabling CXX when GTEST is on, it should be a config error if GTEST is on when CXX is not available, and there should be an appropriate error message.
(Although, perhaps this could be valid for C projects that want to use gtest?)

Can others please weigh in here?

I will note that testing C with GTest is perfectly valid/reasonable. I was tempted to implement it the way you are suggesting, but I didn't want to cause unexpected behavior (i.e. this will break backwards compatibility if people rely on GTest for C codes and haven't been explicitly enabling it)

Was this brought on by an error you encountered?

I think it was this: #105

@white238 yes #105. If you create a C or Fortran only minimal project CMake (at least the latest the latest stable version) throws an error. It’s possible it could be a CMake bug. I didn’t localize it beyond enabling gtest

Sorry coming in to this at the end due to a vacation (Disneyland was fun as always). This looks great.

Like @kennyweiss -- I would prefer we add an error message if ENABLE_GTEST is on, but CXX isn't enabled.

(one that gives folks a chance to know whats wrong vs any cmake error they will encounter otherwise)

Even though you can use gtest for testing c, it's a c++ framework and folks are going to have to use a c++ compiler to compile tests.

kennyweiss · 2017-10-30T17:21:35Z

cmake/BLTOptions.cmake

@@ -93,6 +99,10 @@ if( NOT ( ( BLT_CXX_STD STREQUAL "c++98" )
 Valid Options are ( c++98, c++11, c++14 )")
 endif()

+set(FORTRAN_FORMAT FREE CACHE STRING
+  "Fortran source formatting: FREE (default), FIXED, or INFER")
+set_property(CACHE FORTRAN_FORMAT PROPERTY STRINGS FREE FIXED INFER)


How often do we expect users to want to explicitly set this option?
Should this be marked as an advanced cache variable?

I was surprised and confused that you were setting FREE for everything, and others may have a similar experience. Marking as advanced may delay the resolution of peoples' issues... just food for thought. Personally I think INFER should be the default, but it's your project, so I'll defer to your preferences.

After an internal (offline) discussion, we're leaning towards making INFER the default, and projects that want to change this default can explicitly set the desired behavior.

This seems likely to lead to fewer surprises for fortran projects.

If the default is INFER, would it make sense for FORTRAN_FORMAT to be marked as advanced?

I think it could go either way.

I personally tend to reserve advanced CMake options for ones targeting the
project’s developers most of the time, or ones that are blatantly
experimental or dangerous (like in source builds). Basically anything where
a particular user requested some non standard functionality etc.

In this vein I’m inclined to default any options that a project provides to
non-advanced. In my mind, advanced says “you’re on your own if you mess with
this and we cannot envision a justifiable reason for doing so.” In
practice, however, I find most of the time users don’t even know about
ccmake or cmake-gui and therefore advanced vs normal is of very little
impact.

After reviewing -- I still feel we should simply remove all of the logic related to this and let folks using
BLT set CMAKE_Fortran_FORMAT (or the related source properties) as they see fit in their project.

Adding the option is another thing to explain and support vs standard CMake, which in this case is very straight forward.

Again, sorry for the extra effort, but not having it means we save on docs, maintenance, testing, etc in the long run.

@kennyweiss @white238, what do you think?

The benefit of keeping it is that the code is already written and it would make it easy for people setting up a fortran code to know about this distinction.

The downside is having to describe/maintain it, and the added complexity of additional configuration options.

I'm ok with removing this feature.

zbeekman · 2017-10-30T17:45:24Z

@kennyweiss Since you're waiting on input from colleagues I won't do any major rebasing to cleanup the commits.

I'll fix the message(ERROR ... problem and wait for further direction.

zbeekman · 2017-10-30T20:46:13Z

I'm going to force push some cleaned up commits to my fork. This will update some of the diffs here and cause some of the commentary to go away. If you want me to undo that, just let me know, I've saved a local branch to restore it from.

EDIT: It seems Github has gotten smarter about inline comments/reviews since I last tried that.

zbeekman · 2017-10-30T20:51:09Z

Please let me know if there's anything else I can do to help get this PR to a state where you would be willing to incorporate some or all of the proposed changes. If there are issues with a smaller set of the changes I can factor the PR into smaller one. Your wish is my command! (At least as far as this PR goes)

zbeekman · 2017-10-31T00:23:19Z

EDIT: Comment deleted and moved to correct context:#108 (comment)

white238 · 2017-10-31T00:34:08Z

This looks good, I'll admit I am not great with Fortran so any improvements from a person more knowledgeable than me is always welcome.

zbeekman · 2017-10-31T19:50:39Z

Here is a summary of issues that need to be decided on to merge this PR. If you are happy with the way things are you can check the checkbox, or edit the comment and add your GH name saying so for the purposes of internal discussions. If you check the checkbox I will make the appropriate change and push up to this PR

Default to enabling CXX if GTEST enabled
- Alternate solution: disable GTEST by default UNLESS CXX enabled in project
Leave _languages MATCHES "Fortran"
- vs using less portable IN_LIST (requires CMP0057 NEW and CMake >= 3.3, see Minimum cmake version for blt #109)
Default to letting compiler INFER Fortran source formatting, not making FORTRAN_FORMAT "advanced"
- vs keeping current behavior defaulting to forced FREE formatting for all Fortran sources and/or marking the FORTRAN_FORMAT option advanced.

I'm happy to help out a little bit since resolving this will let me use BLT in a project that I am working on that requires porting from Makefiles/autotools. I can let it sit too, but I want to make sure that anything I can address gets addressed as quickly as it is feasible to do so.

cyrush · 2017-11-03T23:50:42Z

cmake/BLTOptions.cmake

@@ -93,6 +99,10 @@ if( NOT ( ( BLT_CXX_STD STREQUAL "c++98" )
 Valid Options are ( c++98, c++11, c++14 )")
 endif()

+set(FORTRAN_FORMAT FREE CACHE STRING
+  "Fortran source formatting: FREE (default), FIXED, or INFER")
+set_property(CACHE FORTRAN_FORMAT PROPERTY STRINGS FREE FIXED INFER)


After reviewing -- I still feel we should simply remove all of the logic related to this and let folks using
BLT set CMAKE_Fortran_FORMAT (or the related source properties) as they see fit in their project.

Adding the option is another thing to explain and support vs standard CMake, which in this case is very straight forward.

Again, sorry for the extra effort, but not having it means we save on docs, maintenance, testing, etc in the long run.

@kennyweiss @white238, what do you think?

cyrush · 2017-11-03T23:51:23Z

cmake/BLTOptions.cmake

 option(ENABLE_COPY_HEADERS "Copy headers to build directory" OFF)
-option(ENABLE_FORTRAN      "Enables Fortran compiler support" OFF)
+option(ENABLE_FORTRAN      "Enables Fortran compiler support" ${_fortran_already_enabled})
 option(ENABLE_SHARED_LIBS  "Enables shared libraries." OFF)


+1 for ENABLE_FORTRAN defaulting to on when folks already enabled fortran in the project stmt - Thanks for this change!

cyrush · 2017-11-03T23:55:04Z

SetupBLT.cmake

@@ -82,6 +82,13 @@ if (NOT BLT_LOADED)
  endif()

  ################################
+  # Enable CXX required by GTEST
+  ################################
+  if(ENABLE_GTEST)


Like @kennyweiss -- I would prefer we add an error message if ENABLE_GTEST is on, but CXX isn't enabled.

(one that gives folks a chance to know whats wrong vs any cmake error they will encounter otherwise)

Even though you can use gtest for testing c, it's a c++ framework and folks are going to have to use a c++ compiler to compile tests.

white238 · 2017-11-04T00:07:46Z

I agree with @cyrush on the Fortran format... and with the gtest/CXX isssue. If we just turn on the CXX language it means they probably didn't define CMAKE_CXX_COMPILER.

kennyweiss · 2017-11-06T19:34:11Z

Thanks @cyrush
Ok, so, to summarize:

It is a configuration error when ENABLE_GTEST = ON and CXX is not one of the project languages. We need a clear check / error message for this case.
Leave _languages MATCHES "Fortran". We will upgrade to if( ... IN_LIST ... ) when dealing with Minimum cmake version for blt #109
Remove FREE_FORM option from BLT. User codes that want this feature will have to explicity set this outside BLT

Does this work for everyone?

cyrush · 2017-11-06T19:59:30Z

@kennyweiss , yes sounds good to me.

zbeekman · 2017-11-06T20:53:40Z

FWIW, sounds good to me.

…

On Mon, Nov 6, 2017 at 2:59 PM Cyrus Harrison ***@***.***> wrote: @kennyweiss <https://github.com/kennyweiss> , yes sounds good to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#108 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAREPL-cbaKy4oMIz-aCFZz6i04R8ZRUks5sz2UjgaJpZM4QKdgx> .

kennyweiss · 2017-11-10T17:40:59Z

@zbeekman -- are you planning to work on the requested changes, or would you like us to take care of it?

zbeekman · 2017-11-10T17:52:50Z

Yes gonna do it in a sec. busy prepping for SC

…

On Fri, Nov 10, 2017 at 12:41 PM Kenny Weiss ***@***.***> wrote: @zbeekman <https://github.com/zbeekman> -- are you planning to work on the requested changes, or would you like us to take care of it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#108 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAREPNyHPD57wDbOLXXqBeEb0Erh-OP-ks5s1IqsgaJpZM4QKdgx> .

cyrush · 2017-11-10T18:04:26Z

@zbeekman, want to sync up at SC (I'll be there Sun - Wed)?

zbeekman · 2017-11-10T18:08:51Z

Sure, would be great to meet you! My email is ibeekman@paratools.com, let's coordinate that way. I'll be at the PGAS booth a bit too, on the OpenCoarrays poster there.

…

On Fri, Nov 10, 2017 at 1:04 PM Cyrus Harrison ***@***.***> wrote: @zbeekman <https://github.com/zbeekman>, want to sync up at SC (I'll be there Sun - Wed)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#108 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAREPEhOXM5z9WB19wztShyZGUDPxGiCks5s1JArgaJpZM4QKdgx> .

cyrush · 2017-11-10T18:09:24Z

@zbeekman sounds good, I'll ping you via email!

kennyweiss · 2017-11-10T19:48:01Z

Thanks @zbeekman -- no rush at all, just wanted to follow up.

zbeekman · 2017-11-15T22:50:50Z

It looks like the FRUIT smoke tests fail w/ GFortran due to the .f file extension. I'll include a fix.

zbeekman · 2017-11-15T22:53:25Z

Can I just rename blt_fruit_smoke.f to blt_fruit_smoke.f90?

I think that should work for 90% of compilers. This would be the more canonical Fortran convention, rather than explicitly setting free-form source properties in CMake.

kennyweiss · 2017-11-15T22:57:43Z

@zbeekman -- I am fine with the file extension change for this test.

A different strategy would be to set the free-form flag for the fruit smoke test
(with a one-line comment in the cmake file about why you're doing that).

zbeekman · 2017-11-15T23:03:19Z

@kennyweiss yes, I know, but IMO, there are a set of four file extensions allowable for Fortran files (other than include files...that's less clear):

.f: Fixed form, no preprocessing macros
.F: Fixed form, preprocessing required
.f90: Any free form Fortran source file, no preprocessing macros
.F90: Any free form Fortran, preprocessing required

zbeekman · 2017-11-15T23:04:19Z

gah, my gtest fix is broken, stand by...

EDIT: Issue found. I had a typo. I rebased to cleanup the commits and forced pushed to update.

Assuming all tests pass, then I think this is good to merge from my perspective.

- If GTest is enabled without CXX we throw a `FATAL_ERROR` and explain to the user that enabling CXX is a hard requirement for using GTest. - Fixes LLNL#105

- If user explicitly enables Fortran via `project()` or `enable_language()` they should not have to reenable it again.

- Fixes LLNL#106 - Users should explicitly set the `CMAKE_Fortran_FORMAT` vaiable for the project themselves, or set the target property as appropriate.

- Move `blt_fruit_smoke.f` to `blt_fruit_smoke.f90`

cyrush · 2017-11-16T00:16:25Z

@zbeekman renaming sounds like the best solution

cyrush

Looks good, thanks for the changes!

kennyweiss reviewed Oct 30, 2017

View reviewed changes

kennyweiss mentioned this pull request Oct 30, 2017

Minimum cmake version for blt #109

Closed

zbeekman force-pushed the Fortran-Fixes branch from 8fda969 to 065dc44 Compare October 30, 2017 20:47

cyrush self-requested a review November 3, 2017 23:43

cyrush reviewed Nov 3, 2017

View reviewed changes

zbeekman force-pushed the Fortran-Fixes branch 3 times, most recently from 0c01317 to c8d2bde Compare November 15, 2017 22:49

zbeekman added 4 commits November 15, 2017 16:13

Enable GTest by default only when CXX is enabled

29cf834

- If GTest is enabled without CXX we throw a `FATAL_ERROR` and explain to the user that enabling CXX is a hard requirement for using GTest. - Fixes LLNL#105

Default 2 enabling Fortran if use already detected

f236783

- If user explicitly enables Fortran via `project()` or `enable_language()` they should not have to reenable it again.

Remove BLT Fortran format setting

9c8eab1

- Fixes LLNL#106 - Users should explicitly set the `CMAKE_Fortran_FORMAT` vaiable for the project themselves, or set the target property as appropriate.

Use more canonical Fortran file naming convention

428f7d2

- Move `blt_fruit_smoke.f` to `blt_fruit_smoke.f90`

zbeekman force-pushed the Fortran-Fixes branch from 6007f8b to 428f7d2 Compare November 15, 2017 23:14

cyrush approved these changes Nov 16, 2017

View reviewed changes

cyrush merged commit a647ccd into LLNL:master Nov 16, 2017

zbeekman deleted the Fortran-Fixes branch November 28, 2017 20:54

Fortran fixes #108

Fortran fixes #108

Conversation

zbeekman commented Oct 29, 2017 • edited Loading

zbeekman commented Oct 29, 2017

zbeekman commented Oct 29, 2017

kennyweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbeekman Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbeekman Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbeekman commented Oct 30, 2017

zbeekman commented Oct 30, 2017 • edited Loading

zbeekman commented Oct 30, 2017

zbeekman commented Oct 31, 2017 via email • edited Loading

white238 commented Oct 31, 2017

zbeekman commented Oct 31, 2017 • edited by white238 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

white238 commented Nov 4, 2017 • edited Loading

kennyweiss commented Nov 6, 2017

cyrush commented Nov 6, 2017

zbeekman commented Nov 6, 2017 via email

kennyweiss commented Nov 10, 2017

zbeekman commented Nov 10, 2017 via email

cyrush commented Nov 10, 2017

zbeekman commented Nov 10, 2017 via email

cyrush commented Nov 10, 2017

kennyweiss commented Nov 10, 2017

zbeekman commented Nov 15, 2017

zbeekman commented Nov 15, 2017

kennyweiss commented Nov 15, 2017

zbeekman commented Nov 15, 2017

zbeekman commented Nov 15, 2017 • edited Loading

cyrush commented Nov 16, 2017

cyrush left a comment

Choose a reason for hiding this comment

zbeekman commented Oct 29, 2017 •

edited

Loading

zbeekman Oct 30, 2017 •

edited

Loading

zbeekman Oct 30, 2017 •

edited

Loading

zbeekman commented Oct 30, 2017 •

edited

Loading

zbeekman commented Oct 31, 2017 via email •

edited

Loading

zbeekman commented Oct 31, 2017 •

edited by white238

Loading

white238 commented Nov 4, 2017 •

edited

Loading

zbeekman commented Nov 15, 2017 •

edited

Loading