-
Notifications
You must be signed in to change notification settings - Fork 710
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
use proper Python dependency for OTF2 #21325
use proper Python dependency for OTF2 #21325
Conversation
Add Python as a dependency to OTF2 versions to ensure that the toolchain Python is used. To verify, a sanity check is added which ensures that the OTF2 Python module can be loaded. As 'six' is required, newer toolchains also need Python-bundle-PyPI. Signed-off-by: Jan André Reuter <[email protected]>
@boegelbot please test @ jsc-zen3 |
@SebastianAchilles: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2334447417 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot |
@boegelbot please test @ generoso |
@SebastianAchilles: Request for testing this PR well received on login1 PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2334461861 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot |
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.
lgtm
Going in, thanks @Thyre! |
|
||
sanity_check_commands = ['%(namelower)s-config --help'] | ||
sanity_check_commands = ['%(namelower)s-config --help', | ||
'python -c "import %(namelower)s"'] |
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.
I would suggest to use -s
here as otherwise the $HOME packages might be used causing failures/success:
- 'python -c "import %(namelower)s"']
+ 'python -s -c "import %(namelower)s"']
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.
Yes, it makes sense to include that flag, just to make sure that nothing breaks because of user packages. Do you want to make a follow-up PR? I probably don't have the time for that today.
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.
Done: #21362 which also does the same for other existing ECs
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.
@Flamefire This is relevant for several easyblocks too where we use python -c
?
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.
Yes it is
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.
Add Python as a dependency to OTF2 versions to ensure that the toolchain Python is used. To verify, a sanity check is added which ensures that the OTF2 Python module can be loaded. As
six
is required, newer toolchains also need Python-bundle-PyPI.Resolves #21266