reset keep toolchain component class 'constants' every time#1294
reset keep toolchain component class 'constants' every time#1294boegel merged 10 commits intoeasybuilders:developfrom
Conversation
There was a problem hiding this comment.
hmmmm, this is not inheritance-safe. need to think about this one
There was a problem hiding this comment.
maybe
if self._init_LIB_MULTITHREAD is None:
self._init_LIB_MULTITHREAD = self.LIB_MULTITHREAD[:]
else:
self.LIB_MULTITHREAD = self._init_LIB_MULTITHREAD[:]There was a problem hiding this comment.
and add self._init_... = None to the __init__? (or use a dict to store all such non-constant class constants)
There was a problem hiding this comment.
On another note, why do we manually add libiomp5? Using -fopenmp should take care of this?
There was a problem hiding this comment.
should, we had a very good reason at some point for this, and I'm not keen on breaking stuff that works just to remove a handful of characters ;)
There was a problem hiding this comment.
these are libraries you can pass via LDFLAGS, doesn't always work with compiler options. there's probably some crappy code out there that wants library names.
ideally, EB should figure out what libraries -fopenmp links in and use those instead of hardcoding.
There was a problem hiding this comment.
@stdweird: good point on inheritance
How about simply this in __init__.py:
self._init_LIB_MULTITHREAD = self.LIB_MULTITHREAD[:]and this here:
self.LIB_MULTITHREAD = self._init_LIB_MULTITHREAD[:]That should work, no?
Toolchain instances are recreated for different versions.
There was a problem hiding this comment.
then maybe also make 2 private methods _copy_some_class_constants and _restore_some_class_constants and call/extend those.
There was a problem hiding this comment.
I wouldn't generalise this, it should be considered an exception, not encouraged.
|
Refer to this link for build results (access rights to CI server needed): This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
|
Refer to this link for build results (access rights to CI server needed): See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1733/console for more details. Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do. |
|
@stdweird: fixed the inheritance issue you pointed out, please rereview? |
|
Refer to this link for build results (access rights to CI server needed): This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
There was a problem hiding this comment.
no, please hide this in the __init__, people will start to manipultae this
|
Refer to this link for build results (access rights to CI server needed): See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1737/console for more details. Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do. |
|
Refer to this link for build results (access rights to CI server needed): See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1738/console for more details. Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do. |
|
Refer to this link for build results (access rights to CI server needed): This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
|
@stdweird: central solution implemented, unit test enhanced to make sure things work as expected when multiple toolchains and toolchain versions are in play |
|
Refer to this link for build results (access rights to CI server needed): This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
There was a problem hiding this comment.
make a new method
def _init_class_constants(self, names=None):
self.add_class_constants_to_restore(names or [])
self._copy_class_constants()
self._restore_class_constants()
because this looks so strange otherwise
|
although the code looks good, it just feel weird looking at it. but my alternative with the |
|
Refer to this link for build results (access rights to CI server needed): This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
|
The last test report for easybuilders/easybuild-easyconfigs#1337, available at easybuilders/easybuild-easyconfigs#1337 (comment), still shows problems, so I need to look at it more closely, and see why this change isn't fixing the issues there... |
|
retested with easybuilders/easybuild-easyconfigs#1337, works as intended and fixes the problem it intends to fix (previous failing test was due to issues with Intel license server, not due to EB itself) Going in, thanks for the review @stdweird! |
reset keep toolchain component class 'constants' every time
This fixes a problem that often occurs when testing large easyconfig PRs that contain easyconfigs with a large variety on toolchains using
--from-pr, for example easybuilders/easybuild-easyconfigs#1337 (comment) .If old and new version of Intel compilers are used in the same session, things break because constants aren't exactly constant.
This fixes the problem; I took care of checking the other toolchain components too.
@wpoely86, @pforai, @stdweird: please review?