[PATCH] D93778: [test-suite] Support Fortran tests in CMake infrastructure.

[Nichols A. Romero via Phabricator via llvm-commits]

naromero77 added inline comments.


================
Comment at: CMakeLists.txt:7-11
+if (NOT TEST_SUITE_FORTRAN)
+  project(test-suite C CXX)
+else()
+  project(test-suite C CXX Fortran)
+endif()
----------------
Meinersbur wrote:
> Did you consider using `enable_language`? It supports an `OPTIONAL` flag for autodetect the presence of a Fortran compiler. (Not suggesting to use it, it might break buildbots that happen to have gfortran installed, and you will probably not want to run tests for a well-tested distribution provided compiler).
> 
Yes, I looked into that, but the `OPTIONAL` flag does not work at the moment according to the CMake docs.

https://cmake.org/cmake/help/latest/command/enable_language.html




================
Comment at: CMakeLists.txt:30
   CMAKE_C_FLAGS CMAKE_CXX_FLAGS CMAKE_EXE_LINKER_FLAGS)
+if (TEST_SUITE_FORTRAN)
+    mark_as_advanced(CLEAR CMAKE_Fortran_COMPILER)
----------------
Meinersbur wrote:
> [Style] In other places in this file there is no space between `if` and `(`
I will fix this.


================
Comment at: CMakeLists.txt:31
+if (TEST_SUITE_FORTRAN)
+    mark_as_advanced(CLEAR CMAKE_Fortran_COMPILER)
+endif()
----------------
Meinersbur wrote:
> There is probably a reason why the `_COMPILER` vars are marked as advanced, they cannot be changed after the initial configuration. I don't see a reason to make it non-advanced.
Was this just a comment or where there any action you wanted me to to take? 

Here I am just following a parallel constructions similar to the prior `mark_as_advance(...)`.


================
Comment at: CMakeLists.txt:32
+    mark_as_advanced(CLEAR CMAKE_Fortran_COMPILER)
+endif()
 
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Aren't CMAKE variables by convention upper case only?
> Weirdly, cmake defines these as `CMAKE_<Lang>_COMPILER`, where <Lang> is `Fortran`, in Pascal case. Also see https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER.html and explcitly shown at https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS.html.
As @Meinersbur notes, for some reason CMake uses `Fortran` instead of `FORTRAN`.


================
Comment at: CMakeLists.txt:42
+if (TEST_SUITE_FORTRAN)
+  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} ${TEST_SUITE_ARCH_FLAGS}")
+endif()
----------------
Meinersbur wrote:
> [not a change requirest] While good here for consistency with C/CXX, newer Cmake should use [[ https://cmake.org/cmake/help/latest/command/add_compile_definitions.html | add_compile_definitions ]] since `_FLAGS` is supposed to be controlled by the user, and `TEST_SUITE_EXTRA_Fortran_FLAGS` not be necessary.
Thanks for the comments. I noted some other older CMake-isms but did not want to change to much in the first pass.


================
Comment at: CMakeLists.txt:197
     "Extra flags for CMAKE_EXE_LINKER_FLAGS")
+
 mark_as_advanced(TEST_SUITE_EXTRA_C_FLAGS, TEST_SUITE_EXTRA_CXX_FLAGS,
----------------
Meinersbur wrote:
> [nit] unrelated whitespace change
I will fix this.


================
Comment at: SingleSource/UnitTests/Fortran/CMakeLists.txt:1
+file(GLOB Source RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} *.f90 *.F90 *.f *.F)
+
----------------
Meinersbur wrote:
> Isn't this redundant with what `llvm_singlesource` does anyway?
Good question and I didn't know the answer until after I did a little bit of experimentation.

`Source` is an input variable into `llvm_singlesource()`, without it the target is built in the wrong directory and CMake complains.

The structure of `CMakeLists.txt` is similar to what is done elsewhere in the testsuite:
https://github.com/llvm/llvm-test-suite/blob/master/SingleSource/UnitTests/C%2B%2B11/CMakeLists.txt#L4-L9


Repository:
  rT test-suite

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93778/new/

https://reviews.llvm.org/D93778