8000 feat: added support for C++ files compilation by arteevraina · Pull Request #733 · fortran-lang/fpm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Aug 31, 2022
Merged

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

merged 18 commits into from
Aug 31, 2022

Conversation

arteevraina
Copy link
Member
  • Added support for C++ files compilation.
  • Added test for the same.

@arteevraina
Copy link
Member Author
arteevraina commented Aug 27, 2022 •
edited
Loading

cc: @awvwgk @LKedward @jvdp1

LKedward
LKedward reviewed Aug 27, 2022
View reviewed changes
LKedward
LKedward reviewed Aug 27, 2022
View reviewed changes
LKedward
LKedward reviewed Aug 27, 2022
View reviewed changes
Copy link
Member
@LKedward LKedward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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.

LKedward
LKedward reviewed Aug 29, 2022
View reviewed changes
LKedward
LKedward reviewed Aug 29, 2022
View reviewed changes
LKedward
LKedward reviewed Aug 29, 2022
View reviewed changes
Copy link
Member
@LKedward LKedward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
jvdp1 reviewed Aug 29, 2022
View reviewed changes
Copy link
Member
@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LKedward
LKedward reviewed Aug 29, 2022
View reviewed changes
@LKedward
Copy link
Member

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
Copy link
Member

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.

arteevraina and others added 2 commits August 29, 2022 22:11
@arteevraina
refactor: added requested changes
5100827
@LKedward
Update: cpp example package to use cpp stdlib. (#2)
97810e5 
Uses std:vector and std:algorithm to check that
the cpp standard library is linked and called
correctly.
@arteevraina
Copy link
Member Author
arteevraina commented Aug 30, 2022 •
edited
Loading

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
Copy link
Member Author

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
Copy link
Member

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

@LKedward LKedward requested a review from awvwgk August 30, 2022 09:17
awvwgk
awvwgk reviewed Aug 30, 2022
View reviewed changes
src/fpm_targets.f90
Comment on lines +259 to +263
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member
@LKedward LKedward Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++.

LKedward and others added 2 commits August 30, 2022 13:01
@LKedward @awvwgk
Update example_packages/cpp_files/src/cpp_files.f90
24577d1 
Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>
@LKedward
Fix: int type mismatch in cpp example package.
0235f3f
@LKedward
Copy link
Member

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

@zoziha
Copy link
Contributor
zoziha commented Aug 30, 2022 •
edited
Loading

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

zoziha
zoziha approved these changes Aug 31, 2022
View reviewed changes
Copy link
Contributor
@zoziha zoziha left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

awvwgk
awvwgk approved these changes Aug 31, 2022
View reviewed changes
Copy link
Member
@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing, great work.

jvdp1
jvdp1 approved these changes Aug 31, 2022
View reviewed changes
Copy link
Member
@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Thank you @arteevraina

@arteevraina
Copy link
Member Author

Thanks, everyone for the insights on this PR.

@LKedward @jvdp1 @zoziha @awvwgk

@LKedward
Copy link
Member

Great work Arteev @arteevraina - I will now merge.

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

@LKedward LKedward merged commit 2570975 into fortran-lang:main Aug 31, 2022
@arteevraina arteevraina deleted the cpp-support branch August 31, 2022 13:09
@arteevraina
Copy link
Member Author

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 arteevraina mentioned this pull request Sep 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvdp1 jvdp1 jvdp1 approved these changes

@LKedward LKedward LKedward approved these changes

@awvwgk awvwgk awvwgk approved these changes

@zoziha zoziha zoziha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arteevraina @LKedward @zoziha @jvdp1 @awvwgk
0