-
Notifications
You must be signed in to change notification settings - Fork 285
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
reduce the number of command line options for 'cmake' command in CMakeMake generic easyblock #2514
Conversation
bb37039
to
f0047ce
Compare
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) |
@Flamefire conflict resolution needed |
@Flamefire ping about the merge conflicts so we can submit test reports? I'd really like to get this merged :) |
@smoors Rebased, but with no testing as I'm currently OoO, so check if everything looks good. Haven't found any differences in the new diff (before and after force-push) so it should be good. |
Use `' '.join` to add a space only if both config option strings are non-empty
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 30 out of 30 (1 easyconfigs in total) |
Test report by @branfosj Overview of tested easyconfigs (in order)
Build succeeded for 93 out of 95 (10 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
The two failures above are download failures.
Test report by @branfosj Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (1 easyconfigs in total) |
Going in, thanks @Flamefire! |
@Flamefire Shouldn't we check for an existing |
That is already done: https://github.com/easybuilders/easybuild-easyblocks/pull/2514/files#diff-73f272366b8e2ec9f6f8730a2e4b13aca5692579faa17b0f8c9b5d4abf2021a4R202 |
Sorry, I overlooked that... |
setvar('FC', fc) | ||
|
||
# Flags are read from environment variables already since at least CMake 2.8.0 | ||
if LooseVersion(get_software_version('CMake')) < LooseVersion('2.8.0') or cache_exists: |
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.
We're making the assumption here that CMake is indeed listed as a (build) dependency, which was not the case before.
Prior to these changes, the configure step was failing with "cmake: command not found
" (unless cmake
was also available in the OS), but now we get a nasty crash:
ERROR: Traceback (most recent call last):
File "/data/gent/400/vsc40023/easybuild_easy_installed/lib/python3.6/site-packages/easybuild_framework-4.6.1.dev0-py3.6.egg/easybuild/main.py", line 128, in build_and_install_software
(ec_res['success'], app_log, err) = build_and_install_one(ec, init_env)
File "/data/gent/400/vsc40023/easybuild_easy_installed/lib/python3.6/site-packages/easybuild_framework-4.6.1.dev0-py3.6.egg/easybuild/framework/easyblock.py", line 4077, in build_and_install_one
result = app.run_all_steps(run_test_cases=run_test_cases)
File "/data/gent/400/vsc40023/easybuild_easy_installed/lib/python3.6/site-packages/easybuild_framework-4.6.1.dev0-py3.6.egg/easybuild/framework/easyblock.py", line 3960, in run_all_steps
self.run_step(step_name, step_methods)
File "/data/gent/400/vsc40023/easybuild_easy_installed/lib/python3.6/site-packages/easybuild_framework-4.6.1.dev0-py3.6.egg/easybuild/framework/easyblock.py", line 3795, in run_step
step_method(self)()
File "/data/gent/400/vsc40023/easybuild_easy_installed/lib/python3.6/site-packages/easybuild_easyblocks-4.6.1.dev0-py3.6.egg/easybuild/easyblocks/generic/cmakemake.py", line 221, in configure_step
if LooseVersion(get_software_version('CMake')) < LooseVersion('2.8.0') or cache_exists:
File "/usr/lib64/python3.6/distutils/version.py", line 52, in __lt__
c = self._cmp(other)
File "/usr/lib64/python3.6/distutils/version.py", line 335, in _cmp
if self.version == other.version:
AttributeError: 'LooseVersion' object has no attribute 'version'
We're also not taking into account that CMake may be filtered out as a dependency via --filter-deps
(some people may prefer using the CMake that is provided via the OS), so we should have a fallback to determining the CMake version via cmake --version
if get_software_version
returns None
.
@Flamefire Are you up for looking into this? If not, we should open an issue to avoid forgetting about this (we should handle this prior to the next EasyBuild release that includes the changes in this PR)
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.
I looked into this myself, see #2772
Especially to make logs more readable use the following:
Note that CMake reads the environment only during first configure. Hence if CMakeCache.txt exists the args still need to be passed, but that should be very rare as we usually build in a clean dir