-
Notifications
You must be signed in to change notification settings - Fork 305
also install easybuild package for EasyBuild >= 5
#3700
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
also install easybuild package for EasyBuild >= 5
#3700
Conversation
Micket
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.
lgtm
|
@branfosj Can you share some more information on this, explain why this is required? |
I think this was discussed in a conf call, but for clarity this is useful for e.g. easybuilders/easybuild-easyconfigs#22777 |
Some part of the tools developed by @Micket, and his group, require that the I have a concern that we will hit an issue that easybuilders/easybuild-easyconfigs#22774 will break without the changes in this PR, meaning that the EB install latest command line option will not work. I am not sure how to check that though. This may mean that an alternative solution (so that |
ah, I think I see what you mean. That's not really something we want to break :/ |
See #3725 |
concerns over impact on --install-latest-eb-release
|
Solved in #3725 instead |
Co-authored-by: Jasper Grimm <[email protected]>
|
LGTM |
|
Don't merge this yet please, I'm concerned this may cause trouble, for example with exsiting |
|
@boegel was planning to wait until submitting a test report anyway |
|
Test report by @jfgrimm Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) edit: correctly builds as normal, no |
|
successful report using the It correctly installed the |
OK, but what if the |
That works fine. This PR is required if the @jfgrimm test report #3700 (comment) is that case. And I'll have one appear here soon for that case as well. |
|
the only edge case I can think of is that with #3725 merged, we might be overwriting the egg-info file in the case where |
|
Test report by @branfosj Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
|
Test report by @branfosj Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
|
Test report by @branfosj Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) edit |
…cts in constructor of EasyBuildMeta easyblock
| # consider setuptools first, in case it is listed as a sources | ||
| self.easybuild_pkgs.insert(0, 'setuptools') | ||
| elif (LooseVersion(self.version) >= LooseVersion('5.0') and | ||
| any(f'easybuild-{self.version}' in source['filename'] for source in self.cfg['sources'])): |
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.
source may not be a dict, depends on the easyconfig file being used, see EasyBuild-4.9.1.eb for example, so we need be be more careful here: branfosj#2
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 0 out of 1 (1 easyconfigs in total) |
take into account that sources may be a list of strings instead of dicts in constructor of `EasyBuildMeta` easyblock
|
Test report by @jfgrimm Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
|
Test report by @jfgrimm Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) |
boegel
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.
lgtm
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 5 out of 5 (5 easyconfigs in total) |
|
Test report by @branfosj Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) |
|
I've created #3733 as a reminder to remove the egg-info workaround at some future point |
(created using
eb --new-pr)