easybuild-framework icon indicating copy to clipboard operation
easybuild-framework copied to clipboard

Add make_extension_string and _make_extension_list to EasyBlock

Open Flamefire opened this issue 3 years ago • 7 comments

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,

Flamefire avatar May 25 '21 15:05 Flamefire

@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 avatar May 27 '21 07:05 boegel

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 https://github.com/easybuilders/easybuild-easyblocks/pull/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 https://github.com/easybuilders/easybuild-easyblocks/pull/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 avatar May 27 '21 07:05 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?

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

Flamefire avatar Jul 12 '21 14:07 Flamefire

Ok, this is what I had to do to make our use case work again: https://github.com/ComputeCanada/easybuild-easyblocks/commit/7185b5211c2f180c76ecc7bbebcf743fde1501e7

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

mboisson avatar Jul 12 '21 19:07 mboisson

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

Flamefire avatar Jul 26 '22 08:07 Flamefire

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

Flamefire avatar Jul 26 '22 12:07 Flamefire

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

boegel avatar Aug 03 '22 14:08 boegel

Conflicts here

akesandgren avatar Feb 16 '23 12:02 akesandgren

Conflicts here

Not anymore ;-)

Flamefire avatar May 11 '23 07:05 Flamefire

Going in, thanks @Flamefire!

akesandgren avatar May 11 '23 08:05 akesandgren