Skip to content
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.

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

Add -fallow-argument-mismatch to gfortran in MPAS standalone builds #4821

Conversation

xylar
Copy link
Contributor

@xylar xylar commented Mar 1, 2022

This allows the code to build with gfortran >=10.0.0

Related to #4817, but for standalone MPAS builds

[BFB]

@xylar xylar added BFB PR leaves answers BFB mpas-framework GNU GNU compiler related issues labels Mar 1, 2022
@xylar
Copy link
Contributor Author

xylar commented Mar 1, 2022

Testing

With this fix, I was able to build with gfortran 10.3.0, whereas I got errors (see MPAS-Dev/compass#323) without it.

With this fix, I successfully ran the ocean nightly test suite with gfortran 10.3.0.

I will test on Anvil with the older gfortran there to make sure this does no harm.

Update: I ran the ocean pr test suite build with gfortran 8.2.0 on Anvil using this branch and it was bit-for-bit with a baseline using the same version of gfortran and master.

@xylar xylar force-pushed the mpas-framework/fix-standalone-gfortran-10-build branch from 5a2891f to 2092b97 Compare March 1, 2022 16:49
@xylar xylar marked this pull request as draft March 1, 2022 17:39
@xylar
Copy link
Contributor Author

xylar commented Mar 1, 2022

This is not working for gfortran < 10, so I need to work on that.

@xylar xylar force-pushed the mpas-framework/fix-standalone-gfortran-10-build branch 2 times, most recently from 4937211 to 85de14d Compare March 1, 2022 21:41
@xylar xylar requested a review from philipwjones March 1, 2022 21:48
@xylar xylar marked this pull request as ready for review March 1, 2022 21:48
@xylar
Copy link
Contributor Author

xylar commented Mar 1, 2022

I'm still testing this on Anvil but it does at least build with an older gfortran.

Comment on lines 299 to 306
GFORTRAN_GTE_10=$$(expr `gfortran -dumpversion | cut -f1 -d.` \>= 10) ;\
if [ "$${GFORTRAN_GTE_10}" = "1" ]; then \
FFLAGS="-fallow-argument-mismatch"; \
else \
FFLAGS=""; \
fi; \
( $(MAKE) all \
"FC_PARALLEL = mpif90" \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philipwjones, do you know of a more elegant way to do this? If so, I'm keen to learn.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, not really.

@xylar xylar changed the title Add -fallow-argument-mismatch to gfortran Add -fallow-argument-mismatch to gfortran in MPAS standalone builds Mar 2, 2022
Copy link
Contributor

@philipwjones philipwjones left a comment

Choose a reason for hiding this comment

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

You might consider using a more specific FFLAG name just to avoid confusion with the final FFLAGS variable that's created later for all flags.

But I approve regardless. This appears to be needed for gnu builds.

Comment on lines 299 to 306
GFORTRAN_GTE_10=$$(expr `gfortran -dumpversion | cut -f1 -d.` \>= 10) ;\
if [ "$${GFORTRAN_GTE_10}" = "1" ]; then \
FFLAGS="-fallow-argument-mismatch"; \
else \
FFLAGS=""; \
fi; \
( $(MAKE) all \
"FC_PARALLEL = mpif90" \
Copy link
Contributor

Choose a reason for hiding this comment

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

No, not really.

@xylar
Copy link
Contributor Author

xylar commented Mar 2, 2022

You might consider using a more specific FFLAG name just to avoid confusion with the final FFLAGS variable that's created later for all flags.

Thanks @philipwjones, I completely agree. I'll give this a different name.

This allows MPAS to build with gfortran >=10
@xylar xylar force-pushed the mpas-framework/fix-standalone-gfortran-10-build branch from 85de14d to c3ab4e6 Compare March 2, 2022 17:34
@xylar
Copy link
Contributor Author

xylar commented Mar 2, 2022

@philipwjones, I called it EXTRA_FFLAGS, instead of FFLAGS.

@xylar
Copy link
Contributor Author

xylar commented Mar 2, 2022

@mark-petersen, @akturner and @matthewhoffman, this is ready for your review.

@@ -296,6 +296,12 @@ intel-mpi:
"CPPFLAGS = $(MODEL_FORMULATION) -D_MPI" )

gfortran:
GFORTRAN_GTE_10=$$(expr `gfortran -dumpversion | cut -f1 -d.` \>= 10) ;\
if [ "$${GFORTRAN_GTE_10}" = "1" ]; then \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be single dollar sign? My understanding was that $$ accesses shell variables, but single dollar sign should be used for Makefile variables (https://stackoverflow.com/questions/26564825/what-is-the-meaning-of-a-double-dollar-sign-in-bash-makefile). If my understanding is correct, it is interesting that using $$ here still works.

The $$ is also used below on all the lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthewhoffman, my understanding of Makefiles is pretty limited but the single dollar sign version definitely wasn't working.

My understanding it that the make targets are shell scripts, unlike the rest of the make file, and they require $$ to escape the dollar sign so you are actually making shell calls as you want to here.

If you want to try what you suggest and see if you can get it working, be my guest. I wasn't successful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. I tested the makefile but checking for version >=1 so it adds the flag. Compile dies with an error with gnu V6 (as expected) because the flag -fallow-argument-mismatch is undefined. I tried to change the double dollar signs to $(GFORTRAN_GTE_10) and $(EXTRA_FFLAGS) and it didn't work (the variables don't contain the correct values). So @xylar did it right - I just don't understand why these are shell variables rather than makefile variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the explanation and testing. I'm convinced. :)

Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

Tested with gnu V6 on grizzly, and it compiles fine. I also changed to check for version >= 1 and confirmed that the flag is added, since I don't have a V10 compiler to test.

@mark-petersen
Copy link
Contributor

@akturner I compiled and tested above. Should be the same for sea ice, since we all use the same Makefile. Can you test or approve based on my comments? Thanks.

Copy link
Contributor

@akturner akturner left a comment

Choose a reason for hiding this comment

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

Passed MPAS-Seaice build and testing

jonbob added a commit that referenced this pull request Mar 14, 2022
…into next (PR #4821)

Add -fallow-argument-mismatch to gfortran in MPAS standalone builds

This allows the code to build with gfortran >=10.0.0

Related to #4817, but for standalone MPAS builds

[BFB]
@jonbob
Copy link
Contributor

jonbob commented Mar 14, 2022

merged to next

@jonbob jonbob merged commit 6daeba2 into E3SM-Project:master Mar 15, 2022
@jonbob
Copy link
Contributor

jonbob commented Mar 15, 2022

merged to master

@xylar xylar deleted the mpas-framework/fix-standalone-gfortran-10-build branch March 15, 2022 17:17
@xylar
Copy link
Contributor Author

xylar commented Mar 15, 2022

Thanks @jonbob, I really appreciate it!

1 similar comment
@xylar
Copy link
Contributor Author

xylar commented Mar 15, 2022

Thanks @jonbob, I really appreciate it!

jonbob added a commit that referenced this pull request Mar 31, 2022
MPAS standalone: Add gnu >=10 support on Cori

This merge adds the -fallow-argument-mismatch flag to the gnu-nersc
target for MPAS standalone builds. This got missed in #4821.

[BFB]
jonbob added a commit that referenced this pull request Apr 1, 2022
MPAS standalone: Add gnu >=10 support on Cori

This merge adds the -fallow-argument-mismatch flag to the gnu-nersc
target for MPAS standalone builds. This got missed in #4821.

[BFB]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB GNU GNU compiler related issues mpas-framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants