-
Notifications
You must be signed in to change notification settings - Fork 202
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
add make_extension_string
and _make_extension_list
methods to EasyBlock
class, so easyblocks can customize them
#3697
add make_extension_string
and _make_extension_list
methods to EasyBlock
class, so easyblocks can customize them
#3697
Conversation
5c3eb74
to
f18cf4f
Compare
@Flamefire I don't really see the point of letting easyblocks redefine how the list of extensions is included in the generated module file... Do you have a particular use case in mind there? Also, changing the |
In easybuilders/easybuild-easyblocks#2386 we discussed how extensions, which are part of other extensions should be handled. The options discussed were to only include their names (as a string, not a tuple) in the
Yes: Test coverage. I changed the functions responsible for creating the list of extensions (and their versions). Only having 0.0 versions for the relevant test cases means that confusion of version of different extensions and e.g. always using "0.0" (by accident) as the version returned will not be detected. Hence different versions are required to make the test meaningful. Please decide on how to continue and I'll update the PR accordingly. The fallout is easy to fix |
Another use case popped up: In PythonPackage installing a requirements file and then adding the installed stuff to the module. CC @mboisson |
Ok, this is what I had to do to make our use case work again: I can open a PR with the relevant code if it is useful |
10b28c9
to
800a859
Compare
800a859
to
be2cd96
Compare
Any update here? Rebased the changes to develop to (hopefully) fix the test failures. |
7963678
to
83266bd
Compare
Looks like https://sources.easybuild.io is down which is why tests fail |
That's been fixed now (since 2 Aug'22), I've re-triggered the tests |
Conflicts here |
83266bd
to
6cd8195
Compare
This allows custom EasyBlocks to override thise and e.g. add more extensions which should be mentioned in the module. This also unifies the handling of getting and converting those into a string which fixes a bug with e.g. R ECs where extensions may only be a plain string.
6cd8195
to
39c9d36
Compare
Not anymore ;-) |
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 @Flamefire! |
make_extension_string
and _make_extension_list
to EasyBlock
make_extension_string
and _make_extension_list
to EasyBlock
make_extension_string
and _make_extension_list
methods to EasyBlock
class, so easyblocks can customize them
This allows custom EasyBlocks to override thise and e.g. add more extensions which should be mentioned in the module.
This also unifies the handling of getting and converting those into a string which fixes a bug with e.g. R ECs where extensions may only be a plain string.
Before:
setenv("EBEXTSLISTR","b-a,d-a,g-r,g-r,g-r,m-e,s-p,s-t,s-t,t-o,u-t,Rmpi-0.6-9, ...
After:
setenv("EBEXTSLISTR", "base,datasets,graphics,grDevices,grid,methods,splines,stats,stats4,tools,utils,Rmpi-0.6-9,