PSyclone
PSyclone copied to clipboard
1735 simplify using lfric infrastructure
Move the LFRic infrastructure files into subdirectories that are used in LFRic. This makes it easier to replace the included infrastructure with original LFRic files.
This code removed the list of files for compilation testing, instead the new Makefile is used to compile the infrastructure during testing if requested. All Makefiles use a target in the PSycline LFRic infrastructure to get the list if include directories to use, but in tests/lfric_build
it was easier to use Python to query the list of directories. There should be no more hard-coded path.
Codecov Report
Base: 99.42% // Head: 99.50% // Increases project coverage by +0.08%
:tada:
Coverage data is based on head (
bacb2e0
) compared to base (1e64c4b
). Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@
## master #1831 +/- ##
==========================================
+ Coverage 99.42% 99.50% +0.08%
==========================================
Files 262 262
Lines 39043 39054 +11
==========================================
+ Hits 38818 38862 +44
+ Misses 225 192 -33
Impacted Files | Coverage Δ | |
---|---|---|
src/psyclone/tests/conftest.py | 98.55% <100.00%> (ø) |
|
src/psyclone/tests/gocean1p0_stencil_test.py | 100.00% <100.00%> (ø) |
|
src/psyclone/tests/gocean1p0_test.py | 100.00% <100.00%> (ø) |
|
src/psyclone/tests/gocean_build.py | 100.00% <100.00%> (ø) |
|
src/psyclone/tests/lfric_build.py | 100.00% <100.00%> (+3.44%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
This PR:
- Moves the infrastructure files into the same subdirectories as they are in LFRic, making it easier to replace our stub with the real LFRic files.
- It improves the
Makefile
to find all source files in all directories automatically and compile them. The files are like LFRic compiled into the same directories, which means many include paths are now required to compile (why on earth does LFRic do that??? The 'base' name of all files must be unique due to the dependency analysis anyway, so all .o and .mod files could be stored in one flat directory). - It provides an include makefile that other makefiles can use to get the list of include paths to provide (i.e. we can anytime add or remove files and directories without any change anywhere in PSyclone).
- The
lfric_build
class uses this makefile to build the infrastructure files (instead of a pre-defined list of files). - The
lfric_build
class automatically checks for all subdirectories to get the required include list. RE-using the makefile (i.e. callingmake
from python to get the list of include paths this way was really much longer compared with quickly getting all directories). - All other makesfiles (examples, psydata, tutorial) use the include makefile to query the include paths.
- I added tests for
gocean_build
andlfric_build
by allowing the tests to replace the 'make' command with something else (true
orfalse
). Now all the build classes are 100% covered, even in non-compilation tests. - I renamed
gocean1p0_build
togocean_build
...1p0
in the name was just wrong if you ask me :) There was one minor change to this file (making the 'make' command configurable ... a bit annoying, in hindsight I should not have renamed it). But I already had changed dozens of tests to use the new name :(
There are two or three things that I would recommend to make separate PRs for, since this PR is already quite big (though mostly moving files, and minor modifications to many many makefiles):
- Now that gocean and lfric build use
make
, some duplicated code should be moved into the base classCompile
(i.e. triggering make, and checking for errors). I didn't do this here because I renamed gocean_build. - We should consider to support
$F90
and$F90FLAGS
in pytest as well. I had problems with our jenkins builds ... as it turns out in our 'intel' pytests, my team member didn't provide--f90
and--f90flags
to pytest ... as a result intel pytest on jenkins was actually using gfortran, and it all worked because pytest uses the infrastructure files compiled by python calling the compiler ... which was gfortran. With this PRpytest
callsmake
, which picked up the settings in the environment and then compiled the infrastructure with ifort, but all other tests with gfortran 😱 If we change the default for the pytests command line options (f90/f90flags) to be the same as the environment variables if available, we could just modify the compiler with one environment variable consistently everywhere. I've fixed our jenkins, and my PR passed the full compilation test now. - I think with my trick of changing the compilation command from
make
totrue
andfalse
, it should be possible to 100% covertests/utilities.py
, but I suggest leaving this to a separate PR to avoid making this even bigger.
Ready for first review.
One thing I forgot to mention: Ideally we would also replace the hard-coded dependency
file with a rule that recreates this file. But this needs an update to the latest version of fparser (since we need support for dependencies in subdirectories)
Branches passed all tests here as well as our jenkins compilation tests. Ready for next review.
Thanks for the review and debugging. It turns out that by sheer coincidence I always had the infrastructure library inside of PSyclone built, so the incorrect paths that were used inside of the compilation in pytest --compile
never mattered.
While trying to fix this nicely, I found a nice solution to avoid that each process compiles its own infrastructure library (#1849), but I'll leave this for a separate PR, and implement the not-so-nice solution (a conditional setting of a variable in the include file).