-
Notifications
You must be signed in to change notification settings - Fork 295
Refactor LLVM easyblock and install OpenMP library symlinks #3815
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
Conversation
2383711
to
d6a7e63
Compare
- Use f-Strings - Update comments - Use list append/extend
`LooseVersion(self.version)` in `sanity_check_step` can be done only once. Comparisons of a LooseVersion to a string already converts the string.
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
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 3 (3 easyconfigs in total) |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
@boegel @Flamefire At the very least until we have it tested or we have a sanity check to ensure GCCcore ECs do not ship software compiled with OpenMP i would keep them off |
@ocaisa i'm not sure if we discussed about this in an issue? |
I discussed this with @Thyre at #3755 (comment) where the missing libs/symlinks were the only difference of using the Clang vs the LLVM easyblock, so we assumed it was just missed. |
As far as i remember from previous discussion the problems is when using LLVM as the base of a toolchain that reuses GCCcore stuff. |
The original discussion around this is at easybuilders/easybuild-framework#4535 |
Thanks for the link! So from what I understood: With I guess that means we should add an option for this to the easyblock such that clang easyconfigs continue to have it, see easybuilders/easybuild-easyconfigs#23055 |
LLVM is also going to act as a compiler for the toolchain, the problem will be the risk of mixing GCC with LLVM openmp libraries (since we decided to build the new LLVM toolchain on top of GCCcore instead of starting everything from scratch) Also wondering what packages is hard using/checking the name of the OMP library instead of using |
One of the worst offenders is CMake itself, which can directly link the OpenMP libraries instead of using the compiler flags. However, I don't know how they determine the OpenMP library to link. |
Shouldnt't CMake pick up the correct libraries for clang/flang? I think the risk here is always running something built with |
Looks like CMake uses So IMO CMake should be using the compiler libs as it queries the compiler first to see which libs are required and then hard-codes them to the linker invocation |
Based on #3812, see reduced changeset / diff to that
Only functional change: Install the OpenMP alias symlinks which we had before in the Clang easyblock. Doesn't hurt and avoids failures for programs expecting the alternative library names.
The rest are mostly small refactorings pulled out from #3755 to contain non-functional or minor changes.