Update test compatibility matrix #51
Conversation
.github/actions/test-fc/action.yml
Outdated
| [[ "$output" == *"hello world"* ]] && echo "$output" || (echo "Unexpected Fortran program output: $output"; exit 1) | ||
| rm hw | ||
|
|
||
| - name: Test compile (bash) (intentionally failing) |
There was a problem hiding this comment.
git is displaying the diff a bit strangely - I've added this one run command here which moves the library back to its original place to demonstrate the failure. This is to be removed ahead of merging the changes in hw.f90 and setup_fortran.sh
hw.f90
Outdated
| n = count(logical_array) | ||
| allocate(location_array(1:n)) | ||
| location_array = pack(monotone_array, mask=logical_array) |
There was a problem hiding this comment.
This is a sketch of the function that ultimately gave me trouble in my own repo. It's pared down considerably, to the point of not even functioning since monotone_array isn't even initialized, but I found that all this was necessary to replicate the issue, up to and including the pack command (which by itself did not trigger the issue). Putting it into a separate function, as opposed to inlining these commands, was also necessary for replication.
There was a problem hiding this comment.
it might be good to move this new test case to a separate file — in the future we might also think about test compiling a few external programs to cast a wider net for similar issues?
|
Link to the actions in my own repo with the intentionally failing test: https://github.com/nbelakovski/setup-fortran/actions/runs/7277645925 |
|
cc @zaikunzhang |
|
After taking a look at how the windows github runner installs gcc 12 (it just downloads mingw64 and extracts it to /c/mingw64), maybe we should just mv the whole mingw64 folder instead of the current piecemeal approach? |
|
Thanks @nbelakovski for the detailed reproduction and PR.
That seems better than the current approach. Want to edit
Ideally we would test everything but the matrix gets very large, so currently we only test the latest of each toolchain in push/pr CI and do comprehensive testing monthly (discussion) |
|
I've pushed a modification to mv the whole folder.
I needed to add gcc 11 and 12 in order to demonstrate reproducibility of the issue and also demonstrate the fix. Up to you if you'd like to keep them or if you'd like me to remove them, just let me know. As someone trying to fix an issue with a version that's not tested it adds extra effort, and seeing the lack of testing for purportedly supported versions makes me think this is probably not a very robust project and maybe I shouldn't depend on it (I'm not saying that this is true, I'm just saying this is the impression I get when I see the lack of tests). Plus CI minutes are cheap, so why does it matter if the matrix is large? Returning to the PR, if the changes look good, let me know and I'll remove the intentionally failing test (and also remove gcc 11/12 at your direction) and we can merge. |
|
We could test all combinations on PR and the minimal set on push. Or comprehensive on push and minimal on PR — so one can easily CI test on a fork before opening a PR, but we avoid saturating the fortran-lang org |
|
We can run all tests in CI like before, it will just take a little longer if the repo is saturated, but it is still better than skipping a check and risking breakage |
|
@nbelakovski I pulled your branch and updated the test workflow, with your permission I can push and we can get this merged |
|
@wpbonelli I'm not entirely sure what you mean, but yes you have my permission to push? Is the only holdup the removal of the demonstration test case? |
On my own repo I had issues running programs compiled with gfortran 11 in CI. It turned out they were being compiled with gfortran 11, but were picking up libgfortran-5.dll from /c/mingw64/bin, which is from gcc 12, at runtime, and would crash. I've modified hw.90 to reproduce the problem as minimally as I can, although perhaps someone else can make it more minimal. I've temporarily added an intentionally failing action to demonstrate the issue, obviously we would remove that ahead of merging. I also added gcc 12 to the test matrix. It claims to be supported so it seems only logical to test it in addition to 11 and 13.
mv the whole mingw64 folder instead of moving things piecemeal. An investigation of how the windows image installs mingw64 (https://github.com/actions/runner-images/blob/main/images/windows/scripts/build/Install-Mingw64.ps1) shows that it just extracts the whole thing to /c/mingw64, so it should be safe and complete to mv the whole folder.
- remove reproduction test case - comprehensive test on all trigger events - move scripts and test programs to subdirs - move bobyqa test program to separate file - fix testing schedule: weekly not monthly
|
Just figure better to ask before editing someone else's branch even if GitHub allows it. I rebased and tacked on a commit. |
Eh, go ahead and make the changes! I think there's a check mark when you submit the PR as to whether others are allowed to make changes to it. |
|
Any updates? Other than actually running the test program I think this is good to go. |
…_compat.csv, reorganize in .github/compat/, update compatibility files and readme to reflect nvidia support
|
Sorry for the delay here- think it is in good shape now, if you don't mind having a final look? The last commit makes a distinction between testing and reporting mode, on push or PR only known supported runner/toolchain/version combinations are checked, on schedule (now weekly) or manual trigger it tries them all and updates if needed |
|
This PR was for fixing an issue with gcc11, but that doesn't seem to be the main focus of it anymore. I will remove my changes related to gcc11, put them in a separate branch and make a separate PR, and then rename this PR to something more appropriate and let the project maintainers review it, since I am not the appropriate person to review these changes. |
Moving them to a separate branch since this branch has become focused on the test matrix.
nbelakovski
changed the title
|
@nbelakovski Thanks for separating the changesets, that makes things clearer. I made the test matrix changes here to reflect the discussion/decision above to do more frequent comprehensive testing, but it would have been better not to hijack your PR. |
UPDATE: This PR is now about updating the test matrix. A new PR to handle the original issue is now here. The original comments are left as they are.
On my own repo I had issues running programs compiled with gfortran 11 in CI. It turned out they were being compiled with gfortran 11, but were picking up libgfortran-5.dll from /c/mingw64/bin, which is from gcc 12, at runtime, and would crash.
I've modified hw.90 to reproduce the problem as minimally as I can, although perhaps someone else can make it more minimal.
I've temporarily added an intentionally failing action to demonstrate the issue, obviously we would remove that ahead of merging.
I also added gcc 12 to the test matrix. It claims to be supported so it seems only logical to test it in addition to 11 and 13.