feat: added support for C++ files compilation #733

arteevraina · 2022-08-27T06:16:11Z

Added support for C++ files compilation.
Added test for the same.

arteevraina · 2022-08-27T06:17:58Z

src/fpm_compiler.f90

LKedward

Great work @arteevraina - you got that done quickly! All looks to be going in the right direction to me. A few minor comments:

We don't need a new parsing routine parse_cpp_source; we can just reuse the parse_c_source and add logic to set unit_type based on the filename suffix. (So we also don't need a new unit test either.)
We still need to add in a linker flag for the C++ standard library if we do have cpp sources files.
Can you add a new test package with C++ source files and add it to run_tests.sh?
For everyone's information: fpm currently doesn't recompile C files if their include files change (it doesn't track changes to include files), and the same issue will affect cpp files - but that is a separate issue to this PR (see Improve support for include statements #358).

arteevraina · 2022-08-28T13:17:06Z

Great work @arteevraina - you got that done quickly! All looks to be going in the right direction to me. A few minor comments:

* We don't need a new parsing routine `parse_cpp_source`; we can just reuse the `parse_c_source` and add logic to set `unit_type` based on the filename suffix. (So we also don't need a new unit test either.)

* We still need to add in a linker flag for the C++ standard library if we do have cpp sources files.

* Can you add a new test package with C++ source files and add it to [`run_tests.sh`](https://github.com/fortran-lang/fpm/blob/main/ci/run_tests.sh)?

* For everyone's information: fpm currently doesn't recompile C files if their include files change (it doesn't track changes to include files), and the same issue will affect cpp files - but that is a separate issue to this PR (see #358).

Thanks for the review @LKedward. I have addressed the changes mentioned in the comment. Let me know if any further changes are required.

src/fpm_targets.f90

ci/run_tests.sh

LKedward

Looking good @arteevraina - I've left a few minor comments. I might have a think about how we can make the cpp example package more interesting so that it actually using the c++ standard library, perhaps using std::vector or something.

LKedward · 2022-08-29T11:41:28Z

Oh one more thing: can you update example_packages/README.md at some point to explain all the new packages for other maintainers to understand. Thanks!

jvdp1

LGTM. Well done. In addtion to @LKedward 's comments, here are a couple of minor comments.

src/fpm_compiler.f90

src/fpm_sources.f90

src/fpm_targets.f90

LKedward · 2022-08-29T13:47:36Z

Spotted another thing: you need to update this logic to use cpp_compiler_flags for cpp targets:

fpm/src/fpm_targets.f90

Lines 728 to 732 in 97ea8ce

    
           if (target%target_type /= FPM_TARGET_C_OBJECT) then 
        
               target%compile_flags = model%fortran_compile_flags 
        
           else 
        
               target%compile_flags = model%c_compile_flags 
        
           end if

LKedward · 2022-08-29T13:55:25Z

I might have a think about how we can make the cpp example package more interesting so that it actually using the c++ standard library, perhaps using std::vector or something.

I've created a PR to update the cpp example package in arteevraina#2. This can be merged after the above comments have been addressed.

Uses std:vector and std:algorithm to check that the cpp standard library is linked and called correctly.

arteevraina · 2022-08-30T06:35:47Z

Oh one more thing: can you update example_packages/README.md at some point to explain all the new packages for other maintainers to understand. Thanks!

Sure thing, I will add this in a separate Pull Request and update it for all the recently added examples.

arteevraina · 2022-08-30T06:38:06Z

I might have a think about how we can make the cpp example package more interesting so that it actually using the c++ standard library, perhaps using std::vector or something.

I've created a PR to update the cpp example package in arteevraina#2. This can be merged after the above comments have been addressed.

Thanks again for the review @LKedward @jvdp1

I have done the requested changes and also merged your PR here. But, the CI is failing for compile tests for this package in macos.

LKedward · 2022-08-30T08:26:46Z

Great stuff Arteev @arteevraina. I'll take a look at the MacOS issue and see if I can figure out what's going on.

example_packages/cpp_files/src/cpp_files.f90

src/fpm.f90

src/fpm_command_line.f90

src/fpm_sources.f90

awvwgk · 2022-08-30T11:20:22Z

src/fpm_targets.f90

+                        if (get_os_type() == OS_MACOS) then
+                            model%link_libraries = [model%link_libraries, string_t("c++")]
+                        else
+                            model%link_libraries = [model%link_libraries, string_t("stdc++")]
+                        end if


Is this generally true? In case we are using clang++, g++ or icpc on MacOS do we always need -lc++?

Good point. I don't have an answer unfortunately but it's not a blocker for this PR. I'm know very little about the MacOS ecosystem and I don't have a Mac to test these things out. ~~A quick test in the CI shows that it works with clang++ as well as g++ on MacOS-11.~~

Scratch that last sentence, it wasn't actually testing clang++ when I used --cpp-compiler=clang++

Okay, with that fixed I've tried it again and clang++ works on MacOS with -lc++. I can't comment on other compiler combinations, and how supported that is in C++.

Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>

LKedward · 2022-08-30T12:25:34Z

I'm not sure why, but I can't seem to get the --cpp-compiler flag to actually change the cpp compiler.

zoziha · 2022-08-30T13:49:17Z

I'm not sure why, but I can't seem to get the --cpp-compiler flag to actually change the cpp compiler.

It seem be that cpp_compiler has not been updated to fpm_xxx_settings, the patch is as follows:
(Also show cxx compilation INFO in verbose mode)

diff --git a/src/fpm.f90 b/src/fpm.f90
index 497cb6a0..74306501 100644
--- a/src/fpm.f90
+++ b/src/fpm.f90
@@ -216,11 +216,13 @@ subroutine build_model(model, settings, package, error)
     if (allocated(error)) return
 
     if (settings%verbose) then
-        write(*,*)'<INFO> BUILD_NAME: ',model%build_prefix
+        write(*,*)'<INFO> BUILD_NAME:  ',model%build_prefix
         write(*,*)'<INFO> COMPILER:  ',model%compiler%fc
         write(*,*)'<INFO> C COMPILER:  ',model%compiler%cc
+        write(*,*)'<INFO> CXX COMPILER: ',model%compiler%cxx
         write(*,*)'<INFO> COMPILER OPTIONS:  ', model%fortran_compile_flags
         write(*,*)'<INFO> C COMPILER OPTIONS:  ', model%c_compile_flags
+        write(*,*)'<INFO> CXX COMPILER OPTIONS: ', model%cpp_compile_flags
         write(*,*)'<INFO> LINKER OPTIONS:  ', model%link_flags
         write(*,*)'<INFO> INCLUDE DIRECTORIES:  [', string_cat(model%include_dirs,','),']'
      end if
diff --git a/src/fpm_command_line.f90 b/src/fpm_command_line.f90
index 7401f54c..c34931a7 100644
--- a/src/fpm_command_line.f90
+++ b/src/fpm_command_line.f90
@@ -302,6 +302,7 @@ contains
             & prune=.not.lget('no-prune'), &
             & compiler=val_compiler, &
             & c_compiler=c_compiler, &
+            & cpp_compiler=cpp_compiler, &
             & archiver=archiver, &
             & flag=val_flag, &
             & cflag=val_cflag, &
@@ -332,6 +333,7 @@ contains
             & prune=.not.lget('no-prune'), &
             & compiler=val_compiler, &
             & c_compiler=c_compiler, &
+            & cpp_compiler=cpp_compiler, &
             & archiver=archiver, &
             & flag=val_flag, &
             & cflag=val_cflag, &
@@ -488,6 +490,7 @@ contains
                 prune=.not.lget('no-prune'), &
                 compiler=val_compiler, &
                 c_compiler=c_compiler, &
+                cpp_compiler=cpp_compiler, &
                 archiver=archiver, &
                 flag=val_flag, &
                 cflag=val_cflag, &
@@ -545,6 +548,7 @@ contains
             & prune=.not.lget('no-prune'), &
             & compiler=val_compiler, &
             & c_compiler=c_compiler, &
+            & cpp_compiler=cpp_compiler, &
             & archiver=archiver, &
             & flag=val_flag, &
             & cflag=val_cflag, &

Set the cxx compiler and query the cxx compiler in CMake. CMake calls the cxx compiler "CXX compiler". Maybe we need to unify cxx_compiler or cpp_compiler here?

diff --git a/src/fpm.f90 b/src/fpm.f90
index 497cb6a0..74306501 100644
--- a/src/fpm.f90
+++ b/src/fpm.f90
@@ -216,11 +216,13 @@ subroutine build_model(model, settings, package, error)
     if (allocated(error)) return
 
     if (settings%verbose) then
-        write(*,*)'<INFO> BUILD_NAME: ',model%build_prefix
+        write(*,*)'<INFO> BUILD_NAME:  ',model%build_prefix
         write(*,*)'<INFO> COMPILER:  ',model%compiler%fc
         write(*,*)'<INFO> C COMPILER:  ',model%compiler%cc
+        write(*,*)'<INFO> CPP COMPILER: ',model%compiler%cxx
         write(*,*)'<INFO> COMPILER OPTIONS:  ', model%fortran_compile_flags
         write(*,*)'<INFO> C COMPILER OPTIONS:  ', model%c_compile_flags
+        write(*,*)'<INFO> CPP COMPILER OPTIONS: ', model%cpp_compile_flags
         write(*,*)'<INFO> LINKER OPTIONS:  ', model%link_flags
         write(*,*)'<INFO> INCLUDE DIRECTORIES:  [', string_cat(model%include_dirs,','),']'
      end if

…xx at left out places

zoziha

This is really remarkable work, LGTM, thanks @arteevraina .

awvwgk

Thanks for sharing, great work.

jvdp1

Nice work! Thank you @arteevraina

arteevraina · 2022-08-31T11:24:03Z

Thanks, everyone for the insights on this PR.

@LKedward @jvdp1 @zoziha @awvwgk

LKedward · 2022-08-31T12:48:02Z

Great work Arteev @arteevraina - I will now merge.

Some future things to consider for c++ suppport are:

Hashing of included files to detect changes, Improve support for include statements #358
Support for C++ modules

arteevraina · 2022-08-31T14:30:25Z

Great work Arteev @arteevraina - I will now merge.

Some future things to consider for c++ suppport are:
* Hashing of included files to detect changes, [Improve support for include statements #358](https://github.com/fortran-lang/fpm/issues/358)

* Support for C++ modules

Sure thing.

arteevraina added 2 commits August 27, 2022 11:42

feat: added support for cpp files compilation

26468d2

test: added test for cpp_source parsing

9fc44cc

LKedward reviewed Aug 27, 2022

View reviewed changes

src/fpm_compiler.f90 Outdated Show resolved Hide resolved

LKedward reviewed Aug 27, 2022

View reviewed changes

src/fpm_compiler.f90 Outdated Show resolved Hide resolved

LKedward reviewed Aug 27, 2022

View reviewed changes

arteevraina added 8 commits August 28, 2022 18:28

refactor: removed parse_cpp_source subroutine and test for it

08b0815

fix: added link flag if unit type is CPP

8e4e3f9

docs: added example package

8d695c5

chore: added example package to run in ci

ecac6ca

refactor: rename c_compile to cpp_compiler

484d2b3

refactor: rename cppc to cxx

d8de3ce

refactor: cppc to cxx at leftout places

39a304d

refactor: remove boiler plate from fpm.toml of added example

97ea8ce