Skip to content

Rename post_install_step to post_processing_step + deprecate use of post_install_step#4715

Merged
boegel merged 8 commits intoeasybuilders:5.0.xfrom
bartoldeman:post-install-step-rename
Dec 18, 2024
Merged

Rename post_install_step to post_processing_step + deprecate use of post_install_step#4715
boegel merged 8 commits intoeasybuilders:5.0.xfrom
bartoldeman:post-install-step-rename

Conversation

@bartoldeman
Copy link
Copy Markdown
Contributor

This deprecates post_install_step.
Fixes #4656

bartoldeman and others added 4 commits December 4, 2024 18:26
Now we have both libtoy.a and libtoy_post.a, both need moving here.
…uld create an infinite loop because post_install_step calls post_processing_step
Comment thread easybuild/framework/easyblock.py Outdated
self.log.deprecated(
"EasyBlock.post_install_step() is deprecated, use EasyBlock.post_processing_step() instead.",
'6.0',
)
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.

@bartoldeman I had to remove the call to self.post_install_step() which you had here, otherwise we run into an infinite loop when using an easyblock that still defines post_install_step (see the enhanced version of test_post_processing_step which I've pushed in).

I don't think we need to call post_install_step here, and I also think we can/should move the deprecation warning into post_install_step above...

Any particular reason why you didn't take that approach?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was for situations like this:

framework calls post_processing_step
easyblock has post_install_step but does not call super(...

several easyblocks do this. The easyblocks post_install_step needs to be called from somewhere...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so this needs more work. I'll do this with another test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it was actually needed to run stuff inside the easyblock's post_install_step() (which wasn't tested) with or without the super. Super caused the recursion though.
Simplest solution is to keep post_install_step in easyblock.py unmodified from before the PR (!), then post_processing_step calls post_install_step unconditionally with or without deprecation warning.

@boegel's method completely bypassed post_install_step() in easyblocks, so
I've added a simple check to see if it's used (a line printed to stdout).
@boegel boegel changed the title Rename post_install_step to post_processing_step Rename post_install_step to post_processing_step + deprecate use of post_install_step Dec 18, 2024
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

Tested both with easyblocks that still customize post_install_step, and with the updated ones from easybuilders/easybuild-easyblocks#3525 .

When using easyblocks that still use `post_install_stept, it's working as designed producing a deprecation warning:

WARNING: Deprecated functionality, will no longer work in EasyBuild v6.0: EasyBlock.post_install_step() is deprecated, use EasyBlock.post_processing_step() instead.; see https://docs.easybuild.io/deprecated-functionality/ for more information

Works like a charm now, thanks @bartoldeman!

@boegel boegel merged commit abf8fbe into easybuilders:5.0.x Dec 18, 2024
bartoldeman added a commit to ComputeCanada/easybuild-easyblocks that referenced this pull request Apr 5, 2025
bartoldeman added a commit to ComputeCanada/easybuild-easyblocks that referenced this pull request Apr 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants