-
Notifications
You must be signed in to change notification settings - Fork 308
add support for post install commands for Python extensions #2381
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
Conversation
ocaisa
left a comment
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.
Since postinstallcmds is already in framework, I think it would be better to name this option to something like ext_postinstallcmds so that it'll turn up when searching the repo
|
@ocaisa thanks for the suggestions. |
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.
Code LGTM, have you got an easyconfig that uses it?
|
Tested with easybuilders/easybuild-easyconfigs#12559 and looks good, relevant output excerpt below, no impact on other extensions and does what it says on the tin: |
| }, | ||
| ]) | ||
|
|
||
| ext_postinstallcmds = self.cfg['ext_postinstallcmds'] |
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, I wonder if it wouldn't have been better to use the existing postinstallcmds instead...
You can already specify postinstallcmds for a specific extension (just like any other common easyconfig parameter), but doing so won't have any effect, unless a change like this is made in easyblocks that are used to install extensions.
So, how about just renaming to ext_postinstallcmds to postinstallcmds here after all (and removing the support for picking up ext_postinstallcmds, while this is still only in develop?
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.
@boegel that would indeed be better.
I went for another parameter to avoid the clash with the global postinstallcmds, but it's easy to fix this, by adding this line to the __init__ function in extensionblock.py (similar to the start_dir parameter):
self.cfg['postinstallcmds'] = self.ext.get('options', {}).get('postinstallcmds', None)
because of the clash I assumed this was intended, but now I understand it was just not yet implemented.
I can make a PR for this if that's the way to go.
post install commands can be done with a hack if using
python setup.py install:with pip however, this hack is not possible.
this PR adds support for post install commands for each individual extension, so one can use it in the same way as the usual
postinstallcmds: