This is an archive of the discontinued LLVM Phabricator instance.

Differential D139633

Enable Fortran testsuite when TEST_SUITE_SUBDIRS=Fortran
ClosedPublic

Authored by pscoro on Dec 8 2022, 7:50 AM.

Details

Reviewers
rzurob
anhtuyen
daltenty
Commits
rTde2e530ed27d: Reland "Enable Fortran testsuite/language when TEST_SUITE_SUBDIRS=Fortran is…
rT8c18b1433474: Enable Fortran testsuite when TEST_SUITE_SUBDIRS=Fortran
Summary

Enable Fortran testsuite/language when TEST_SUITE_SUBDIRS=Fortran is passed

Diff Detail

Event Timeline

pscoro created this revision.Dec 8 2022, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 7:50 AM
pscoro requested review of this revision.Dec 8 2022, 7:50 AM
Harbormaster completed remote builds in B201965: Diff 481277.Dec 8 2022, 7:50 AM
pscoro edited the summary of this revision. (Show Details)Dec 8 2022, 7:53 AM
pscoro added reviewers: rzurob, anhtuyen.
pscoro updated this revision to Diff 481282.Dec 8 2022, 8:06 AM

Removed unneeded secondary enable_language

Harbormaster completed remote builds in B201968: Diff 481282.Dec 8 2022, 8:06 AM
daltenty added a subscriber: daltenty.Dec 12 2022, 7:40 AM
daltenty added inline comments.
CMakeLists.txt
8–13

We still need to enable fortran on the path where the user didn't pass the directory but did toggle the option flag, so I think we should keep that block around in it's current form.

A second minor nit, the pattern we usually use for this in the LLVM CMake is slightly different, we usually just set a var to contain the desired default.

pscoro updated this revision to Diff 482128.Dec 12 2022, 7:50 AM

Addressed Nits

Harbormaster completed remote builds in B202582: Diff 482128.Dec 12 2022, 7:50 AM
daltenty accepted this revision.Dec 12 2022, 12:58 PM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 12 2022, 12:58 PM
daltenty retitled this revision from Enable Fortran testsuite in CMakeLists to Enable Fortran testsuite when TEST_SUITE_SUBDIRS=Fortran.Dec 12 2022, 12:59 PM
Closed by commit rT8c18b1433474: Enable Fortran testsuite when TEST_SUITE_SUBDIRS=Fortran (authored by pscoro, committed by daltenty). · Explain WhyDec 12 2022, 1:43 PM
This revision was automatically updated to reflect the committed changes.
daltenty added a commit: rT8c18b1433474: Enable Fortran testsuite when TEST_SUITE_SUBDIRS=Fortran.
saghir added a reverting change: rTfe925ab05d2f: Revert "Enable Fortran testsuite when TEST_SUITE_SUBDIRS=Fortran".Dec 13 2022, 12:20 PM
pscoro reopened this revision.Dec 14 2022, 10:21 AM

Re-opening to apply fixes and reland

This revision is now accepted and ready to land.Dec 14 2022, 10:21 AM
pscoro updated this revision to Diff 482919.Dec 14 2022, 10:21 AM

Fix for when multiple testsuites passed

Harbormaster completed remote builds in B203168: Diff 482919.Dec 14 2022, 10:21 AM
pscoro updated this revision to Diff 482937.Dec 14 2022, 11:03 AM

fix tested to work

Harbormaster completed remote builds in B203177: Diff 482937.Dec 14 2022, 11:03 AM
pscoro updated this revision to Diff 486548.Jan 5 2023, 6:20 AM

Added set CMAKE_Fortran_COMPILER

Harbormaster completed remote builds in B205890: Diff 486548.Jan 5 2023, 6:20 AM
daltenty added inline comments.Jan 6 2023, 8:25 AM
CMakeLists.txt
10

The docs say this should be the default CMake behaviour: https://cmake.org/cmake/help/latest/envvar/FC.html
so I'm not sure we need to handle this explicitly?

pscoro updated this revision to Diff 486919.Jan 6 2023, 10:05 AM

Removed set CMAKE_Fortran_COMPILER

Harbormaster completed remote builds in B206148: Diff 486919.Jan 6 2023, 10:05 AM
daltenty accepted this revision.Jan 6 2023, 10:50 AM

LGTM with new IN_LIST update

Closed by commit rTde2e530ed27d: Reland "Enable Fortran testsuite/language when TEST_SUITE_SUBDIRS=Fortran is… (authored by pscoro, committed by daltenty). · Explain WhyJan 6 2023, 11:45 AM
This revision was automatically updated to reflect the committed changes.
daltenty added a commit: rTde2e530ed27d: Reland "Enable Fortran testsuite/language when TEST_SUITE_SUBDIRS=Fortran is….

Revision Contents

PathSize
6 lines
Fortran/
UnitTests/
fcvs21_f95/
3 lines

Diff 481277

CMakeLists.txt

cmake_minimum_required(VERSION 3.13.4) cmake_minimum_required(VERSION 3.13.4)
include(CheckSymbolExists) include(CheckSymbolExists)
option(TEST_SUITE_FORTRAN "Enable Fortran test suite" OFF)
project(test-suite C CXX) project(test-suite C CXX)
if(TEST_SUITE_FORTRAN) if(${TEST_SUITE_SUBDIRS} MATCHES "Fortran")
option(TEST_SUITE_FORTRAN "Enable Fortran test suite" ON)
enable_language(Fortran) enable_language(Fortran)
daltentyUnsubmitted
Not Done
ReplyInline Actions

The docs say this should be the default CMake behaviour: https://cmake.org/cmake/help/latest/envvar/FC.html
so I'm not sure we need to handle this explicitly?

daltenty: The docs say this should be the default CMake behaviour: https://cmake.
else()
option(TEST_SUITE_FORTRAN "Enable Fortran test suite" OFF)
endif() endif()
daltentyUnsubmitted
Not Done
ReplyInline Actions
project(test-suite C CXX)
if(${TEST_SUITE_SUBDIRS} MATCHES "Fortran")
- option(TEST_SUITE_FORTRAN "Enable Fortran test suite" ON)
- enable_language(Fortran)
+ set(TEST_SUITE_FORTRAN_default ON)
else()
- option(TEST_SUITE_FORTRAN "Enable Fortran test suite" OFF)
+ set(TEST_SUITE_FORTRAN_default OFF)
endif()
+ option(TEST_SUITE_FORTRAN "Enable Fortran test suite" ${TEST_SUITE_FORTRAN_default})
+ if(TEST_SUITE_FORTRAN)
+ enable_language(Fortran)
+ endif()
function(append value)

We still need to enable fortran on the path where the user didn't pass the directory but did toggle the option flag, so I think we should keep that block around in it's current form.

A second minor nit, the pattern we usually use for this in the LLVM CMake is slightly different, we usually just set a var to contain the desired default.

daltenty: We still need to enable fortran on the path where the user didn't pass the directory but did…
function(append value) function(append value)
foreach(variable ${ARGN}) foreach(variable ${ARGN})
set(${variable} "${${variable}} ${value}" PARENT_SCOPE) set(${variable} "${${variable}} ${value}" PARENT_SCOPE)
endforeach(variable) endforeach(variable)
endfunction() endfunction()
# The test-suite is designed to be built in release mode anyway and # The test-suite is designed to be built in release mode anyway and
▲ Show 20 Lines • Show All 377 Lines • Show Last 20 Lines

Fortran/UnitTests/fcvs21_f95/CMakeLists.txt

Show All 27 Lines
# #
# Tests make use of arithmetic IF which is now a deleted feature # Tests make use of arithmetic IF which is now a deleted feature
# in Fortran 2008 # in Fortran 2008
# #
# Note that the NIST license allows redistribution but requires # Note that the NIST license allows redistribution but requires
# explicit acknowledgement of what has been changed. Please # explicit acknowledgement of what has been changed. Please
# amend README file if you modify the tests. # amend README file if you modify the tests.
include(CheckFortranCompilerFlag)
enable_language(Fortran)
# clean-up fort.* files otherwise one of the tests will fail # clean-up fort.* files otherwise one of the tests will fail
llvm_test_prepare(rm -f %S/fort.*) llvm_test_prepare(rm -f %S/fort.*)
# Different compilers or different compiler options may produce slightly # Different compilers or different compiler options may produce slightly
# different but still valid outputs for some of these tests. By default, we will # different but still valid outputs for some of these tests. By default, we will
# check that the outputs match the reference outputs exactly. However, when this # check that the outputs match the reference outputs exactly. However, when this
# option is set, we will check less strictly. # option is set, we will check less strictly.
option(FCVS_ALLOW_FLEXIBLE_OUTPUT "Don't match the output of FCVS tests exactly") option(FCVS_ALLOW_FLEXIBLE_OUTPUT "Don't match the output of FCVS tests exactly")
▲ Show 20 Lines • Show All 55 Lines • Show Last 20 Lines