-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add option to install "plain OMNI" to install script and upgrade Github actions runners #408
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/408/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #408 +/- ##
==========================================
+ Coverage 95.57% 96.03% +0.45%
==========================================
Files 201 197 -4
Lines 39810 38977 -833
==========================================
- Hits 38050 37430 -620
+ Misses 1760 1547 -213
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
I agree with the approach. And while I'm sad to see the original CLAW go, I think it's time to let go if this unlocks us going forward.
I left one tiny pointer, but otherwise, GTG from me.
@@ -61,6 +62,7 @@ print_usage_with_options() { | |||
echo " --with[out]-jdk Install JDK instead of using system version (default: use system version)" | |||
echo " --with[out]-ant Install ant instead of using system version (default: use system version)" | |||
echo " --with[out]-claw Install CLAW and OMNI Compiler (default: disabled)" | |||
echo " --with[out]-omni Install OMNI Compiler; cannot be combined with --with-claw (default: disabled)" |
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.
Maybe prefer that one in the INSTALL.md
now in the example with --hpc2020
?
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.
Thanks, I have update the instructions and also added the missing ability to install now OMNI instead of CLAW+OMNI via CMake. Please have another look
fa0049e
to
c4dfc2b
Compare
c4dfc2b
to
04dd86b
Compare
Very nice! CMake additions look good; still GTG for me. |
Farewell, old fried! 🫡 😢 https://media1.tenor.com/m/mPHvERT5iKgAAAAC/toy-story-buzz-lightyear.gif |
This is partially in response to #406: It looks like we can keep OMNI alive for the time being (yay...?!) if we give up the specific version bundled with the CLAW, and instead update to the latest upstream. The way this is implemented does not yet remove CLAW entirely, instead a new
--with-omni
option is added to theinstall
script and the existing--with-claw
option is kept as is. The two cannot be combined but using--with-omni
gives the newer F_Front version.Notably, for our purpose it is even sufficient to use the xcodeml-tools component of the OMNI compiler, which provides the frontends.
With this we are able to use recent GNU versions to compile OMNI and thus can upgrade the Github runners to the latest Ubuntu image, which uses GNU 13.2.0 as default. This builds cleanly and all tests seem to work as expected. It should also resolve the spurious errors observed in #230, which I will rebase on top of this PR.
The one downside of these changes is that they means we are stopping the testing of the CLAW variants for CLOUDSC regression tests. But since they don't have any perspective anyway, I think this is acceptable - please shout if you disagree!
I used the opportunity to also update the module versions for HPC2020 to recent versions.
Piggy-backed are also a few fixes where no tmp_path was specified for xmods in tests.