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

Use GCC-9 and GCC-10 in Github Action #94

Merged
merged 1 commit into from
Jan 12, 2021
Merged

Use GCC-9 and GCC-10 in Github Action #94

merged 1 commit into from
Jan 12, 2021

Conversation

kgerheiser
Copy link
Contributor

Add GCC-10 to build matrix

Fixes #93

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

Looks good.

@jbathegit
Copy link
Collaborator

jbathegit commented Jan 11, 2021

Thanks very much for this @kgerheiser , but I'm a little surprised by one thing here. Specifically, it looks like you had a successful gcc-10 build even though you're testing on the develop branch, whereas @mathomp4 wasn't able to do that, and which is why he opened #81 in the first place. In other words, you're not testing on the jba_mstabs branch that I created to fix that issue that he reported. So now I'm curious how you were able to do that, or what you may be doing differently from what he was doing?

@mathomp4, were you doing your own compilation outside of our CMake scripts, perhaps with some different compiler flags than we use in our standard builds? Again, I'm wondering why/how @kgerheiser apparently had no problems with the GNU v10+ compiler even though he wasn't testing against my new branch(?)

@kgerheiser
Copy link
Contributor Author

Yes, building with GCC 10 just worked for me. I didn't really look at the original error.

@mathomp4
Copy link

Yes, building with GCC 10 just worked for me. I didn't really look at the original error.

It's possible @kgerheiser builds bufr with DYNAMIC_ALLOCATION? Or perhaps that is the default in whatever version he has? If so, you avoid the bug.

@jbathegit
Copy link
Collaborator

OK, then it sounds like maybe we need to add testing for STATIC as well as DYNAMIC builds, in order to try to reproduce this error. I hadn't realized this before, but it looks like the static builds are only being tested for the Intel compiler!? Here's a snippet from test/CMakeLists.txt:

# Only Intel supports STATIC_ALLOCATION
if(CMAKE_C_COMPILER_ID MATCHES "^(Intel)$")
  list(APPEND test_kinds "4")
endif()

Does anyone know why this filter is here? Unless I'm missing something, we should be testing all builds (both STATIC and DYNAMIC) for all compilers, not just Intel. The only exceptions are test_OUT_3.f and test_OUT_4.f, which can only be run on DYNAMIC builds because they specifically test features of dynamic allocation. But I can't think of any reason why we shouldn't be testing all of the other STATIC builds on all compilers.

Please understand that when we're talking about STATIC vs. DYNAMIC here, we're talking about how arrays are allocated internally within the library, not about how the library is linked to an application program! So again, at least in my mind there's no reason we shouldn't be testing the STATIC builds as well as the DYNAMIC builds on all platforms (again, for all test programs except for test_OUT_3.f and test_OUT_4.f ;-)

@jbathegit
Copy link
Collaborator

Also, @kgerheiser, I noticed that there are two checks from yesterday which are still hung in a "Waiting for status to be reported" state. Do you have any idea what's going on here?

@kgerheiser
Copy link
Contributor Author

Yes, since the checks changed, they need to be updated in the repository settings

@kgerheiser
Copy link
Contributor Author

I've updated the branch settings. It can be merged now. I think we should merge this and then deal with the other issues separately.

@jbathegit
Copy link
Collaborator

OK, I'll go ahead and merge your main.yml change, and then we can deal with the static vs. dynamic as a separate issue.

@jbathegit jbathegit merged commit 4eb0826 into NOAA-EMC:develop Jan 12, 2021
This pull request was closed.
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.

Test with gfortran 10
4 participants