Honor sanity_check_commands & sanity_check_paths from extensions
#5012
Conversation
Flamefire
force-pushed
the
extension-sanity-check-commands
branch
from
8743828 to
5943e1d
Compare
Flamefire
force-pushed
the
extension-sanity-check-commands
branch
2 times, most recently
from
e1fd987 to
4b556af
Compare
|
I would really like to see #5015 merged first so we can more easily review this... |
Flamefire
force-pushed
the
extension-sanity-check-commands
branch
from
f4b578f to
18417ad
Compare
|
Rebased after merge of that PR |
| '6.0', | ||
| ) | ||
| if extension != self.is_extension: | ||
| raise EasyBuildError('Unexpected value for `extension` argument. ' |
There was a problem hiding this comment.
This isn't exactly ignoring it, as is stated above...
What's the point of raising an error for an incorrect value being passed that's going to be ignored?!
There was a problem hiding this comment.
Because if we have different values it is an error somewhere.
So a) it doesn't need to be passed but b) if it is it should match the expected value
We can remove that check in EB 6 but I'd better play it safe until then
There was a problem hiding this comment.
But why care if the value is different, if we're going to ignore the value anyway?
I sort of see your point, but we can't log a deprecation message claiming we stop caring about extension, only to raise an error based on its value immediately afterwards...
There was a problem hiding this comment.
we can't log a deprecation message claiming we stop caring about
extension, only to raise an error based on its value immediately afterwards...
Because these are 2 separate checks:
- Passing
extensionis allowed, but deprecated - Passing a wrong
extensionis an error
What would you suggest for the wording? Just remove "and will be ignored"?
| # (only relevant when this extension is installed stand-alone via ExtensionEasyBlock) | ||
| self.sanity_check_fail_msgs.append(fail_msg) | ||
|
|
||
| return res |
There was a problem hiding this comment.
Drive-by partial review: why is it OK to change this, and make Extension.sanity_check_step not return anything anymore? This is an API change...
There was a problem hiding this comment.
I misunderstood part of the EasyBlock code because ExtensionEasyBlock got complicated as we now call sanity_check_step from Extension and EasyBlock and the most straight forward way to combine those was to just check sanity_check_fail_msgs because EasyBlock.sanity_check_step doesn't return anything.
But in any case Extension.sanity_check_step and hence ExtensionEasyBlock needs to return this tuple (or actually True/False would be enough)
Flamefire
force-pushed
the
extension-sanity-check-commands
branch
from
724bf28 to
6da2c63
Compare
|
Rebased on develop to investigate and fix prior CI failure |
Flamefire
force-pushed
the
extension-sanity-check-commands
branch
from
6da2c63 to
eda13d6
Compare
This now calls the inherited `EasyBlock.sanity_check_step` that handles the `sanity_check_commands` & `sanity_check_paths` for extensions too.
It is redundant as `self.is_extension` provides that information already.
Flamefire
force-pushed
the
extension-sanity-check-commands
branch
from
eda13d6 to
46eee7d
Compare
2 participants
Currently those are silently ignored leading to success where it should fail.
The only official easyconfig affected by this so far is
MDAnalysis-2.4.2-foss-2021a.ebbut I ran into this while creating an easyconfigIncludes
saved_envcontext manager for restoring environment in tests #5015