-
Notifications
You must be signed in to change notification settings - Fork 295
use property for gcc_prefix/gcc_root in LLVM easyblock #3793
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
d57c49b
to
dc7c566
Compare
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 2 out of 3 (3 easyconfigs in total) |
@Flamefire Merge conflict to fix now that #3817 is merged? |
Rebased |
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) |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) |
easybuild/easyblocks/l/llvm.py
Outdated
|
||
def _set_gcc_prefix_probs(self): | ||
"""Set properties of currently loaded GCC installation""" | ||
self._gcc_root, self._gcc_prefix = self._get_gcc_prefix() |
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 would put this inside an if as it was done originally.
I do not foresee any situation where these variables would change during the build process
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 is a method only used on access of the properties to set the "backing storage". So the if is outside this function
I agree they won't change, so there is no harm if this method is (wrongly) called again
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.
There is no if
either inside gcc_prefix
nor gcc_root
so the method self._get_gcc_prefix()
will be called every time one of these 2 properties are requested
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.
You are right, thanks! Seems I couldn't decide where to put the if
and forgot it in the end.
Added
@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 3051630099 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 4 out of 4 (4 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 this uses properties that auto-initialize on (first) use to avoid them being used before setting in
_set_gcc_prefix
which acted as a configure-method AND setting those propertiesBUGFIX (Separated out to #3817 but would cause a conflict)
For
bootstrap = False
the function_create_compiler_config_file
was called without checking the version which results in adding a CLI argument for clang compilers that was introduced in LLVM 19 which causes failures.