Skip to content

deprecate passing extension option to sanity_check_step & sanity_check_load_module methods in EasyBlock class - #5220

Merged
boegel merged 6 commits into
easybuilders:developfrom
Flamefire:deprecate-extension-param
Jul 29, 2026
Merged

deprecate passing extension option to sanity_check_step & sanity_check_load_module methods in EasyBlock class#5220
boegel merged 6 commits into
easybuilders:developfrom
Flamefire:deprecate-extension-param

Conversation

@Flamefire

@Flamefire Flamefire commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

This removes the superflous parameter which is always set to self.extension anyway.
It looks like an artifact from when that member didn't exist or was not set consistently.

I did this in a PR prior to the 5.3.0 release but took out to get that PR merged without this change.
Reason is to avoid potential mismatches, e.g. by forgetting to pass the parameter.
Ultimately I want to use self.is_extension to allow multiple extension to fail their sanity check step and having a "parallel" source of truth makes that harder.
The cause here is that the failing exts_filter allows to check other extensions but failing commands does not as that aborts directly. Easy to fix when relying on self.is_extension

I'll also removed it from sanity_check_load_module which is similarly simple but requires an update to the easyblocks that use it.

@Flamefire
Flamefire force-pushed the deprecate-extension-param branch from 22abb12 to 397039a Compare July 21, 2026 15:40
@Flamefire
Flamefire force-pushed the deprecate-extension-param branch from 397039a to b9c42ea Compare July 21, 2026 15:45
@Flamefire
Flamefire force-pushed the deprecate-extension-param branch from b9c42ea to aff1182 Compare July 21, 2026 16:33
This is not fully reliable as accidentally passing a non-list as the 2nd
param where `extra_modules` was intended will go silently wrong.
@boegel boegel added the change label Jul 29, 2026

# skip loading of fake module when using --sanity-check-only, load real module instead
if build_option('sanity_check_only') and not extension:
if build_option('sanity_check_only'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this still check self.extension?

Suggested change
if build_option('sanity_check_only'):
if build_option('sanity_check_only') and not self.extension:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, no, because of the inline return above...

Comment thread easybuild/framework/easyblock.py Outdated
@boegel

boegel commented Jul 29, 2026

Copy link
Copy Markdown
Member

Looks OK to me.

I dug into the history a bit:

  • self.is_extension was first introduced in e68bc97 (Jan'13, EasyBuild v1.11.0), but only in ExtensionEasyBlock class, so it could not be used generally;
  • later, in 85f17b6 (Mar'19, EasyBuild v3.9.0), a change was made to ensure that self.is_extension is always defined in EasyBlock class, so since then we can always rely on it, but by then we had the extension option is various methods to effectively pass down the is_extension class parameter

So, makes sense to me to clean this up as proposed.

@boegel boegel added this to the next release (5.3.2?) milestone Jul 29, 2026
@boegel boegel changed the title Deprecate passing extension to sanity_check_step & sanity_check_load_module deprecate passing extension option to sanity_check_step & sanity_check_load_module methods in EasyBlock class Jul 29, 2026

@boegel boegel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@boegel
boegel merged commit 4a9e01f into easybuilders:develop Jul 29, 2026
41 checks passed
@Flamefire
Flamefire deleted the deprecate-extension-param branch July 29, 2026 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants