Skip to content

skip validation when copying EasyConfig object for extension#3071

Merged
akesandgren merged 2 commits intoeasybuilders:developfrom
boegel:skip_validation_exts
Nov 6, 2019
Merged

skip validation when copying EasyConfig object for extension#3071
akesandgren merged 2 commits intoeasybuilders:developfrom
boegel:skip_validation_exts

Conversation

@boegel
Copy link
Member

@boegel boegel commented Nov 5, 2019

Currently when an extension is initialized, the EasyConfig instance of the "parent" is copied into the extension (since stuff like toolchain etc. is also relevant for installing the extension).

By default, this results in re-validating the easyconfig file (which includes re-checking the OS dependencies), which is silly.
Even worse, it leads to problems like the ones reported in easybuilders/easybuild-easyconfigs#8661 since commands like rpm -q may break when particular modules (like XZ are loaded).

cc @zao, @terjekv, @akesandgren

@boegel boegel added the bug fix label Nov 5, 2019
@boegel boegel added this to the next release (4.0.2?) milestone Nov 5, 2019
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.

Yeah, this looks sane. Is that the only place cfg.copy is used?

@boegel
Copy link
Member Author

boegel commented Nov 5, 2019

@akesandgren Basically, yes.

I'd like to get this covered in the tests somehow too, need to think about how to do that exactly...

@terjekv
Copy link
Contributor

terjekv commented Nov 5, 2019

This resolves the issues I've seen with OS dependencies on RHEL8. Thanks!

…ped when copying parent EasyConfig object for extensions
@boegel
Copy link
Member Author

boegel commented Nov 6, 2019

Thanks a lot for confirming the fix @terjekv!

I've enhanced one of the tests to try and ensure that we don't accidentally reverse this fix, which makes the PR good to go...

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 @boegel!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants