Skip to content
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

Merged
merged 1 commit into from May 11, 2023

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented May 25, 2021

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,

@boegel
Copy link
Member

boegel commented May 27, 2021

@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 barbar test extension version is causing fallout in tests that were recently updated (that's why I pushed a sync with develop into this PR). Do you have a particular reason for that change? I think the 0.0 highlight nicely that it's not a real software release...

@boegel boegel modified the milestones: 4.4.0, release after 4.4.0 May 27, 2021
@Flamefire
Copy link
Contributor Author

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?

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 exts_list similar how it is done for R and its "system extensions". This is what the above PR does, but this was rejected. An other suggestion was to allow extensions with a list of names, but that is very intrusive. The last idea was to include an EC param which lists additional extensions instead. Those can be sanity-checked (more or less manually in the EasyBlock) but they need to be added to the list of extensions in the module file, which is the reason for this PR: To provide a way for the easyblock to add those extensions not in exts_list
TBH: I prefer my original approach from easybuilders/easybuild-easyblocks#2386

Do you have a particular reason for that change? I think the 0.0 highlight nicely that it's not a real software release...

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.
Related: That change from ls to ulimit due to the bug with using 2 letters of the name instead of name-version, so something with more than 2 letters should be used.

Please decide on how to continue and I'll update the PR accordingly. The fallout is easy to fix

@Flamefire
Copy link
Contributor Author

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?

Another use case popped up: In PythonPackage installing a requirements file and then adding the installed stuff to the module. CC @mboisson

@mboisson
Copy link
Contributor

Ok, this is what I had to do to make our use case work again:
ComputeCanada/easybuild-easyblocks@7185b52

I can open a PR with the relevant code if it is useful

@boegel boegel modified the milestones: 4.4.2, release after 4.4.2 Sep 2, 2021
@easybuilders easybuilders deleted a comment from boegelbot Sep 29, 2021
@boegel boegel modified the milestones: 4.5.0, 4.x Oct 13, 2021
@Flamefire
Copy link
Contributor Author

Any update here? Rebased the changes to develop to (hopefully) fix the test failures.

@Flamefire
Copy link
Contributor Author

Looks like https://sources.easybuild.io is down which is why tests fail

@easybuilders easybuilders deleted a comment from boegelbot Aug 3, 2022
@boegel
Copy link
Member

boegel commented Aug 3, 2022

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

@akesandgren
Copy link
Contributor

Conflicts here

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.
@Flamefire
Copy link
Contributor Author

Conflicts here

Not anymore ;-)

Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@akesandgren
Copy link
Contributor

Going in, thanks @Flamefire!

@akesandgren akesandgren merged commit 2436f4d into easybuilders:develop May 11, 2023
@Flamefire Flamefire deleted the add_ext_list_function branch May 11, 2023 08:49
@boegel boegel modified the milestones: 4.x, next release (4.7.2) May 17, 2023
@boegel boegel changed the title Add make_extension_string and _make_extension_list to EasyBlock Add make_extension_string and _make_extension_list to EasyBlock May 17, 2023
@boegel boegel changed the title Add make_extension_string and _make_extension_list to EasyBlock add make_extension_string and _make_extension_list methods to EasyBlock class, so easyblocks can customize them May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants