-
Notifications
You must be signed in to change notification settings - Fork 21
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 dynamic memory allocation for all builds, and remove _DA suffix #155
Conversation
I would recommend adding |
Thanks @kgerheiser for the suggestion and PR! So the failure(s) are now saying: |
I'm wondering why did that work previously, but not now? |
Wow this is a considerable simplification! Nice work. When we get down to only one library it will be even simpler... |
I did some more digging and found two lines in |
So now the main.yml tasks are working, but the main_new.yml tasks (for the code coverage) aren't working, which is the exact opposite of what we saw before. Here's what the upload-artifact step is showing:
@edwardhartnett or @aerorahul , any ideas? If we can figure this out then maybe we can also close #135 ;-) |
Interestingly, the code coverage task for the push process just finished successfully, even though it failed previously on the pull_request process. Does that make any sense? Maybe just a temporary connection glitch? If you want I can try rerunning the checks(?) |
So I went ahead and re-ran the failed check, and it now runs to completion successfully. Go figure!? |
Hi @edwardhartnett and @aerorahul. Could at least one of you please review this, and either approve or let me know if you have any further suggestions for improvement? I've also added @jack-woollen in case he may be able to help. Just FYI, I've got a separate update ready which should take care of the few remaining GNU compiler warnings for this library, but I'd like to get this PR squared away first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, although I did not review each file carefully.
In a future PR, why not just move all .f
to .f90
and where appropriate .F
to .F90
or .f90
if there is no preprocessor involved.
No one is using g77
to compile this code anymore.
Fixes #77
This is a prerequisite for task #78, so that we eventually only have to build one version of the library. As part of this update, a number of *.F routines no longer require any preprocessing and therefore have now been moved to *.f or *.f90 names.
The problem I'm having is that all of the Python tests are failing, but that's a known issue that pre-dates this PR (see #135) and is therefore unrelated to these changes.
@aerorahul could you please help me to figure out why the Python tests are no longer working?