-
Notifications
You must be signed in to change notification settings - Fork 295
Don't install libomp aliases by default in LLVM easyblock #3825
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
Don't install libomp aliases by default in LLVM easyblock #3825
Conversation
As discussed in easybuilders/easybuild-framework#4535 it could lead to issues caused by incompatible libraries used by different compilers. Especially GCC and LLVM/Clang picking up the wrong `libomp` vs `libgomp`.
I am fine with it being optional, but i still do not think that having it on |
IMO that is a separate issue: If we don't do that while moving the (existing) Clang easyconfigs to this easyblock we'd change the resulting installation which I'd avoid. |
I would agree with @Flamefire here, we shouldn't change existing behaviour unless we see a problem in real life. The thing is, we don't really know if this problem exists for our existing recipes. The best thing to do would be to scan the jsc-zen3 GCCcore-based installations and see if there are examples of linking against I expect the number of cases to be very small. Even in the cases where it does exist, it may be better to turn OpenMP off rather than switch toolchain to |
I tried to gather a few results here: easybuilders/easybuild-framework#4535 (comment) |
Ship has sailed on EasyBuild v5.1.1, so this will have to wait until release after that (likely end of August) |
Uff. That means we did introduce a behavior change that might affect users although analysis seems not fully conclusive. |
I don't see this as a huge deal-breaker, as we had this situation with Clang for a long time (and Intel compilers as well IIRC). We didn't get a complaint, so fixing this finally in EasyBuild 5.1.1-next looks fine for me. |
Should we then change the default here to |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 3 (3 easyconfigs in total) |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) |
We would still have to switch the ECs to have I would leave the default to |
I meant:
So we have 2 changes without changes to the (ours and users) easyconfigs. Either we merge this for 5.1.1 and keep the no-symlinks-behavior without any changes over the versions or we merge this for 5.1.2 but change the default to True so there is only 1 change from now to 5.1.1 but none from 5.1.1 to 5.1.2 |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) |
@boegelbot please test @ jsc-zen3 |
@Crivella: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 3043921119 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) |
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! |
As discussed in
easybuilders/easybuild-framework#4535 it could lead to issues caused by incompatible libraries used by different compilers. Especially GCC and LLVM/Clang picking up the wrong
libomp
vslibgomp
.See also #3755 (comment) & #3815 (comment)
This restores the behavior changed by #3815 but adds an option to enable them (off by default). This is useful/required for Clang easyconfigs using this easyblock as otherwise there would be a difference compared to using the Clang easyblock. See #3755 (comment)
I used
build_openmp_library_aliases
as the option name to group it nicely with the other OMP options, although "install" is more correct.As there now is a new check for an incompatible combination of options dependent on OpenMP I combined them into a single place and error with correct exit code.
I also made the use of OFF/ON in the CMake arguments consistent as I had to look up whether the checks are case sensitive or not. They are not, so the change is only cosmetic but avoids further confusion.
cc @Thyre @Crivella