-
Notifications
You must be signed in to change notification settings - Fork 295
various fixes for the LLVM easyblock #3706
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
@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 2842451216 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) |
Test report by @Crivella Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) NOTES
|
Did one test locally with 20.1.4 and no bootstrap trying to minimize the build options as much as possible:
but this lead to a new error in all offload tests:
for some reason linking with offloading and the GNU linker requires I am wondering for this if we should:
The error being related to |
Given that See #3703. But yeah, I remember having issues with old ROCm versions, which expected to have / find |
# TODO: Find a better way to either force the test to use the non wrapped compiler or to pass the flags | ||
if build_option('rpath'): | ||
setvar('CFLAGS', "%s %s" % (old_cflags, '-Wno-unused-command-line-argument')) | ||
setvar('CXXFLAGS', "%s %s" % (old_cxxflags, '-Wno-unused-command-line-argument')) |
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.
This part is removed in favor of using the LLVM-19.1.7_clang_rpathwrap_test.patch
in the easyconfigs
Test report by @Crivella Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) NOTES
DETAILS
TEST results
|
Will try and rerun the previous test with everything but |
Test report by @Crivella Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) NOTES
DETAILSTEST reports
|
Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 0 out of 1 (1 easyconfigs in total) |
@Thyre 64c219a should fix #3706 (comment) |
Reverted fix for #3711 as the old project structure was still available (somehow i was using the Also reinstated the old way of detecting the host triple in case the configure step was not called to allow runs with |
Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) Used commit 64c219a |
Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) Used commit 0214115 |
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.
Some minor comments. Looks good to me otherwise.
res = run_shell_cmd(cmd, fail_on_error=False) | ||
# Observed in 20.1.0, the build of the offloading tools can fail due to 'cstdint' file not found | ||
# But will succeed if executed again with -j 1 (possible missing dependency in the CMake logic?) | ||
# See https://github.com/llvm/llvm-project/issues/130783 | ||
if res.exit_code != EasyBuildExit.SUCCESS: | ||
self.log.warning("Build failed, attempting again with parallel ON") | ||
res = run_shell_cmd(cmd, fail_on_error=False) | ||
if res.exit_code != EasyBuildExit.SUCCESS: | ||
self.log.warning("Build failed, attempting again with parallel OFF") | ||
cmd = "make -j 1 VERBOSE=1" | ||
res = run_shell_cmd(cmd) |
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.
Like mentioned in the issue, switching to Ninja might also be an option, though we would need to be careful with the parallelism used by Ninja...
Something to think about for a separate PR I think.
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.
Agree but i think that would definitily be a standalone PRs requiring testing with every configuration of ECs (also would not be easy to check and reproduce the error as it is basically a race condition)
I would recommend that we do a test without any CUDA compute capability or |
Co-authored-by: Jan André Reuter <[email protected]>
5dae3f1
to
17a5217
Compare
Rebased onto develop to fix merge conflict after Changes to `llvm.py` from the rebasediff --git a/easybuild/easyblocks/l/llvm.py b/easybuild/easyblocks/l/llvm.py
index e1a5d65ce..2c64df21f 100644
--- a/easybuild/easyblocks/l/llvm.py
+++ b/easybuild/easyblocks/l/llvm.py
@@ -232,7 +232,7 @@ class EB_LLVM(CMakeMake):
def __init__(self, *args, **kwargs):
"""Initialize LLVM-specific variables."""
- super(EB_LLVM, self).__init__(*args, **kwargs)
+ super().__init__(*args, **kwargs)
self.llvm_src_dir = None
self.llvm_obj_dir_stage1 = None
@@ -684,7 +684,7 @@ class EB_LLVM(CMakeMake):
self.add_cmake_opts()
src_dir = os.path.join(self.llvm_src_dir, 'llvm')
- output = super(EB_LLVM, self).configure_step(builddir=self.llvm_obj_dir_stage1, srcdir=src_dir)
+ output = super().configure_step(builddir=self.llvm_obj_dir_stage1, srcdir=src_dir)
# Get the LLVM HOST TRIPLE (e.g. x86_64-unknown-linux-gnu) from the output
for line in output.splitlines():
@@ -916,7 +916,7 @@ class EB_LLVM(CMakeMake):
print_msg("Building stage 1/1")
change_dir(self.llvm_obj_dir_stage1)
- super(EB_LLVM, self).build_step(*args, **kwargs)
+ super().build_step(*args, **kwargs)
if self.cfg['bootstrap']:
self.log.info("Building stage 2")
@@ -1052,7 +1052,7 @@ class EB_LLVM(CMakeMake):
self.cfg.update('preinstallopts', f'LD_LIBRARY_PATH={lib_path}')
- super(EB_LLVM, self).install_step()
+ super().install_step()
# copy Python bindings here in post-install step so that it is not done more than once in multi_deps context
if self.cfg['python_bindings']:
@@ -1329,7 +1329,7 @@ class EB_LLVM(CMakeMake):
else:
self._sanity_check_gcc_prefix(gcc_prefix_compilers, self.gcc_prefix, self.installdir)
- return super(EB_LLVM, self).sanity_check_step(custom_paths=custom_paths, custom_commands=custom_commands)
+ return super().sanity_check_step(custom_paths=custom_paths, custom_commands=custom_commands)
def make_module_step(self, *args, **kwargs):
"""
@@ -1353,7 +1353,7 @@ class EB_LLVM(CMakeMake):
def make_module_extra(self):
"""Custom variables for Clang module."""
- txt = super(EB_LLVM, self).make_module_extra()
+ txt = super().make_module_extra()
# we set the symbolizer path so that asan/tsan give meanfull output by default
asan_symbolizer_path = os.path.join(self.installdir, 'bin', 'llvm-symbolizer')
txt += self.module_generator.set_environment('ASAN_SYMBOLIZER_PATH', asan_symbolizer_path) |
Tested again with one GCC version for every major release of LLVM for 18, 19, 20
The SYSTEM ones have the problem with the CI check, but I would not let it block this PR I think now bot the ECs and the EBlock are good to go, and would try to merge them so the work on the toolchain can start being implemented (As mentioned in easybuilders/easybuild-easyconfigs#22887 there is also 20.1.5 released but it still does not have all the release files uploaded, in particular |
@Thyre If you have time, can you give a review on this, you are far more capable than me to do that (I'm happy to do a readthrough once you give the ok) |
Sure! I'll do another pass hopefully by the end of tomorrow. I'll give you a ping once I'm finished. |
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.
Minor comments regarding the comments in the EasyBlock. Otherwise LGTM.
There are two minor things we may want to improve in a separate PR, but are both just minor improvements that should not hold back this PR:
|
Co-authored-by: Jan André Reuter <[email protected]>
Co-authored-by: Jan André Reuter <[email protected]>
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
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.
Read through and just a minor comment
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.
Builds have been tested in other PRs, and in particular easybuilders/easybuild-easyconfigs#22903 |
Fixes issues:
--rebuild
failures #3708self.make_installdir()
in theprepare_step
saniy_check_step
skipping checks for.so
files #3710>=20.1.2
will fail due to changes in the project folder structure #3711_prepare_runtimes_rpath_wrappers
method that can be ran also at the first build stepbootstrap = False
20.1.4
build (~minimal): various fixes for the LLVM easyblock #3706 (comment)bootstrap = False
20.1.4
build (all components): various fixes for the LLVM easyblock #3706 (comment)glob
ing for the runtimes directory after the first buildOther changes:
GPU
andignore_patterns
for testsSPIRV
moved to a non-experimental target (this might need to be enabled in a version specific way as this happened for versions>=20.x