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

Revert changes to configure.defaults from commit 48f7904d #254

Closed
wants to merge 1 commit into from

Conversation

mgduda
Copy link
Collaborator

@mgduda mgduda commented Jun 7, 2024

This PR reverts changes to configure.defaults from commit 48f7904.

The reorganization in configure.defaults from commit 48f7904 breaks compilation with the classic build system (configure/compile/clean) with GNU compilers.

In the configure script, logic to determine whether we need to add -fallow-argument-mismatch relies on the extraction of the verbatim value of FFLAGS for use in compiling a test program. Commit 48f7904 introduced Make variables into FFLAGS and these variables aren't expanded when extracting FFLAGS in the configure script, leading to an invalid set of compiler flags for the gnu_flag_test.F90 test program.

The reorganization in configure.defaults from commit 48f7904 breaks compilation
with the classic build system (configure/compile/clean) with GNU compilers.

In the configure script, logic to determine whether we need to add
-fallow-argument-mismatch relies on the extraction of the verbatim value of
FFLAGS for use in compiling a test program. Since commit 48f7904 introduced
Make variables into FFLAGS, these variables aren't expanded when extracting
FFLAGS in the configure script, leading to an invalid set of compiler flags for
the gnu_flag_test.F90 test program.
@mgduda mgduda requested a review from islas June 7, 2024 20:28
@islas
Copy link
Contributor

islas commented Jun 7, 2024

I think the only flag needed for that test program should be the free-form flags, thus :

FFLAGS=`grep ^FORMAT_FREE configure.wps | cut -d"=" -f2-`

should be sufficient

@islas
Copy link
Contributor

islas commented Jun 8, 2024

@mgduda Do you think that my suggestion would be a reasonable fix?

@mgduda
Copy link
Collaborator Author

mgduda commented Jun 10, 2024

@mgduda Do you think that my suggestion would be a reasonable fix?

There are a couple of other places in the configure script where we extract FFLAGS from the configure.wps file, and I think just getting FORMAT_FREE would work in those cases, too. However, I think it's generally better if we use the full set of flags that are specified in a stanza when compiling test programs.

I'll put together a PR with changes to extract FFLAGS and FORMAT_FREE, and to combine them for use in the configure script.

@islas
Copy link
Contributor

islas commented Jun 10, 2024

However, I think it's generally better if we use the full set of flags that are specified in a stanza when compiling test programs.

I can see that it might seem beneficial to do that, but I think we shouldn't. I think if you are testing for one feature, then you should only use the flags necessary to test that feature. Otherwise, you run the risk of false negatives where other flags inadvertently cause failure and you instead mark that feature as not supported or worse fail configuration with a totally wrong diagnosis.

Case in point, in WRF when checking nc4 capabilities, there were numerous ways to fail the check with things unrelated to the bare minimum netCDF flags. One such example is the HDF5 linking (wrf-model/WRF#1935) that isn't needed for testing this feature. Thus, if a user does not have HDF5 in path (though netCDF can provide via RPATH) this feature would be marked as failure.

@mgduda
Copy link
Collaborator Author

mgduda commented Jun 10, 2024

@islas Can we say with certainty that the only compiler flags that are needed for all test programs in the configure script are those that are captured in what is now the FORMAT_FREE variable for all configuration stanzas? If not, then I would argue that the safest approach is a solution that results in the previous behavior prior to the merge of PR #230, i.e., to use all specified compiler flags when building test programs.

I'm not disagreeing that the only necessary flags for the gnu_flag_test.F90 test program should be free-format-related flags, but (1) we need to consider that we aren't directly able to test all configuration stanzas, and (2) there are more test programs than just gnu_flag_test.F90 for which we need to extract compiler flags in the configure script.

@islas
Copy link
Contributor

islas commented Jun 10, 2024

No, I can't say whether all test programs for all stanzas FORMAT_FREE would suffice. I do think the safest approach is as you said, however I am also suggesting for gnu_flag_test.F90, as well as for any other test program the best solution would be to only ever use the smallest necessary subset of flags to verify that feature respectively.

That is probably not possible or within the scope of this fix, and as you noted many other test programs also utilize FFLAGS so fixing this to behave as before is probably the most reasonable solution.

My comment was more to note that this style of testing compiler features with a full set of flags is something we should consider avoiding in the future (not this PR). Maybe my wording was too strong suggesting "...I think we shouldn't..." in this PR vs in general. I do think even with a fix to reconstruct all the FFLAGS correctly, gnu_flag_test.F90 should only pull the FORMAT_FREE section.

@mgduda
Copy link
Collaborator Author

mgduda commented Jun 11, 2024

The reversion of changes from 48f7904 has been obviated by PR #255.

@mgduda mgduda closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants