-
Notifications
You must be signed in to change notification settings - Fork 217
Add --keep-going option to fail at the end not at first failing installation
#5022
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
base: develop
Are you sure you want to change the base?
Conversation
Allows to install any possible easyconfig without stopping if a single one fails
Co-authored-by: Jan André Reuter <[email protected]>
|
Here's the output for a very simple test, trying to install Intel compilers on aarch64 and some other EasyConfig at the same time: Used command: Output: Click to openSo this is working as expected. Might come in very handy in preparing 2025a on this system for PR testing... |
|
There are errors which this doesn't catch. Mainly, I've encountered these two so far: The second case is arbitrarily made up, but came up due to the EasyStack I used being created with an EasyConfig not existing upstream. Don't know how easy it would be to catch such cases as well, since they also abort This honestly isn't a blocker though, as one can still use flags to work around the former and should make sure that the files exist for the latter. |
|
I think having no or missing easyconfigs should error in any case as it might hint that you mistyped an option or similar issue where it might not do what you expected. And we can argue that this option is documented to continue on a failed build and if it fails to parse it is a different issue. |
Absolutely, a missing EasyConfig should error out in any case. I'm fine with keeping the current behavior for all three ( |
IIRC we have And failing to parse an easyconfig could as well be that you accidentally passed an easyblock or patch instead of an easyconfig, so again it isn't a build issue which we want to ignore with this option. If this can be made more clear in the description we could do that. But as it fails right at the start I think it is fine as-is. |
easybuild/main.py
Outdated
| if options.dump_test_report or options.upload_test_report: | ||
| # Generation test reports is successful even when software failed to build | ||
| return EasyBuildExit.SUCCESS |
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.
Hmm, not sure about this...
Exit code should always indicate whether (all) installations were successful or not, regardless of whether a test report was uploaded/dumped? That seems like the least surprising behavior to me...
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.
Shouldn't the exit code indicate an error in the requested operation? And a failed installation while explicitly testing for those isn't an error.
So it's a definition issue and I'm not fully sure how to solve it, so either way is fine to me. Hence I'm changing this to an error as suggested
|
@Flamefire merge conflict to fix... |
|
Resolved |
Allows to install any possible easyconfig without stopping if a single one fails
The main function will call
sys.exitwith the returned exit code as-if failing by anEasyBuildErrorsoeb --keep-going ec1.eb ec2-ebwill return an error code that can be used in scripts