8000 bug: C preprocessor does not propagate directives to executables by gnikit · Pull Request #775 · fortran-lang/fpm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

bug: C preprocessor does not propagate directives to executables #775

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 4 commits into from
Oct 13, 2022
Merged

bug: C preprocessor does not propagate directives to executables #775

merged 4 commits into from
Oct 13, 2022

Conversation

gnikit
Copy link
Member
@gnikit gnikit commented Oct 10, 2022

Fixes #774

@gnikit gnikit requested a review from LKedward October 10, 2022 10:48
gnikit added a commit to gnikit/metis-fpm that referenced this pull request Oct 10, 2022
@gnikit
feat: add programs
6161c2b 
FYI fpm has a bug see fortran-lang/fpm#775
where C preprocessor flags do not propagate into
the executable. The way to circumvent this is by
passing both --flag and --c-flag options during run/build/etc.
LKedward
LKedward approved these changes Oct 10, 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.

Thanks for the quick fix @gnikit! This looks good to me 🚀

@LKedward LKedward requested a review from zoziha October 10, 2022 15:59
zoziha
zoziha approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor
@zoziha zoziha left a comment

Choose a reason for hiding this comment

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

8000

Thanks for sharing @gnikit .

awvwgk
awvwgk reviewed Oct 12, 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.

Can we have one of the example packages testing this be behavior?

@gnikit
Copy link
Member Author
gnikit commented Oct 12, 2022

Can we have one of the example packages testing this be behavior?

ok I'll push an example

@gnikit
Copy link
Member Author
gnikit commented Oct 12, 2022

@awvwgk The existing c_main example in main can replicate this, since it incorrectly calls gfortran instead of gcc and thus generates a series of warnings

 + mkdir -p build/dependencies
 <INFO> BUILD_NAME: build/gfortran
 <INFO> COMPILER:  gfortran
 <INFO> C COMPILER:  gcc
 <INFO> CXX COMPILER: g++
 <INFO> COMPILER OPTIONS:   -Wall -Wextra -Wimplicit-interface -fPIC -fmax-errors=1 -g -fcheck=bounds -fcheck=array-temps -fbacktrace -fcoarray=single
 <INFO> C COMPILER OPTIONS:  
 <INFO> CXX COMPILER OPTIONS: 
 <INFO> LINKER OPTIONS:  
 <INFO> INCLUDE DIRECTORIES:  []
 + mkdir -p build/gfortran_2A42023B310FA28D
[  0%]                         main.c
 + mkdir -p build/gfortran_2A42023B310FA28D/c-main/
 + gfortran -c app/main.c  -Wall -Wextra -Wimplicit-interface -fPIC -fmax-errors=1 -g -fcheck=bounds -fcheck=array-temps -fbacktrace -fcoarray=single -J build/gfortran_2A42023B310FA28D -Ibuild/gfortran_2A42023B310FA28D -o build/gfortran_2A42023B310FA28D/c-main/app_main.c.o
cc1: warning: command-line option ‘-Wimplicit-interface’ is valid for Fortran but not for C
cc1: warning: command-line option ‘-fcheck=array-temps’ is valid for Fortran but not for C
cc1: warning: command-line option ‘-fbacktrace’ is valid for Fortran but not for C
cc1: warning: command-line option ‘-fcoarray=single’ is valid for Fortran but not for C
[ 50%]                         main.c  done.
[ 50%]                         c-main
 + mkdir -p build/gfortran_2A42023B310FA28D/app/
 + gfortran  -Wall -Wextra -Wimpli
8000
cit-interface -fPIC -fmax-errors=1 -g -fcheck=bounds -fcheck=array-temps -fbacktrace -fcoarray=single  build/gfortran_2A42023B310FA28D/c-main/app_main.c.o  -o build/gfortran_2A42023B310FA28D/app/c-main
[100%]                         c-main  done.
[100%] Project compiled successfully.
 + build/gfortran_2A42023B310FA28D/app/c-main

@zoziha
Copy link
Contributor
zoziha commented Oct 12, 2022

For example, setting a c-flag with macro definitions to ensure that main.c is compiled correctly? If the c-flag is not correctly applied to main.c, will the compilation report an error?

@gnikit
Copy link
Member Author
gnikit commented Oct 12, 2022

For example, setting a c-flag with macro definitions to ensure that main.c is compiled correctly? If the c-flag is not correctly applied to main.c, will the compilation report an error?

Sure we can add something like that. Do the examples run as part of our test suite?

@zoziha
Copy link
Contributor
zoziha commented Oct 12, 2022

Yes, we can refer to add_example_package and add_example_to_run_in_ci, these example packages are declared and run in ci/run_tests.sh, which will start the test at GitHub Actions.

@zoziha
Copy link
Contributor
zoziha commented Oct 12, 2022 •
edited
Loading

It seems that because there are no object files that need to be packaged into libc_main_preprocess.a, the ar usage on macOS is wrong, which may be a bug of fpm and causes CI to fail.

@gnikit
Copy link
Member Author
gnikit commented Oct 12, 2022

It seems that because there are no object files that need to be packaged into libc_main_preprocess.a, the ar usage on macOS is wrong, which may be a bug of fpm.

Yes, potentially. Another potential bug is that if you add preprocessor.cpp macros in the config file, fpm does not pick those up for C. I will file this separately

@gnikit
Copy link
Member Author
gnikit commented Oct 12, 2022

@zoziha For now I might just add an empty stub .c file to circumvent this

8000
@gnikit gnikit force-pushed the gnikit/issue774 branch from bc4b878 to 9d0b31d Compare October 12, 2022 10:24
@zoziha
Copy link
Contributor
zoziha commented Oct 12, 2022 •
edited
Loading

Maybe moving val.h to the include folder (which is the default C-language header folder for fpm) can temporarily fix the problem, and remove the include-dir='src' statement in fpm.toml.
(See my temp commit and passed CI)

Of course, your solution should also work @gnikit .

@gnikit
Copy link
Member Author
gnikit commented Oct 12, 2022

Maybe moving val.h to the include folder (which is the default C-language header folder for fpm) can temporarily fix the problem, and remove the include-dir='src' statement in fpm.toml.

Whatever you want, I'm okay either way.

@gnikit
Copy link
Member Author
gnikit commented Oct 12, 2022

If everyone is happy with this you can go ahead and merge.

I would leave my test setup as it is just so we can have commit 9d0b31d in the history to inspect why the OS X CI failed and whether it's an fpm bug.

@gnikit gnikit mentioned this pull request Oct 12, 2022
Closed
@LKedward LKedward merged commit 49f2ff3 into main Oct 13, 2022
@LKedward LKedward deleted the gnikit/issue774 branch October 13, 2022 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LKedward LKedward LKedward approved these changes

@awvwgk awvwgk awvwgk left review comments

@arteevraina arteevraina arteevraina left review comments

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

bug: C preprocessor does not propagate directives to executables
5 participants
@gnikit @zoziha @LKedward @awvwgk @arteevraina
0