-
Notifications
You must be signed in to change notification settings - Fork 203
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
enhance systemtools.py to also support POWER systems #1044
Conversation
@wpoely86: please review? |
@@ -219,30 +222,40 @@ def get_cpu_speed(): | |||
try: |
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.
seeing this block made me think of http://www.isitfriday.org/ on a thursday....
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.
why is this whole block wrapped in a try/except
?
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.
please recheck, cleaned up significantly now
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.
The affinity mask is a POSIX thing right? So why not try the BSD thing and then fall back to the affinity mask?
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 don't think the BSD one takes into account cpuset and all that crap, the affinity one does, so that should be tried first (and it should always work on Linux)
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.
yeah agreed. But now, it's only tried on Linux. Why not try it always?
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.
because it won't work on non-Linux systems :)
I wholeheartedly endorse this change (given my need to get building on PPC64). 👍 |
Note: If you could add "PPC = 'PPC'" somewhere around line 55 that would make it easier to add the following to toolchains/compiler/gcc.py: COMPILER_OPTIMAL_ARCHITECTURE_OPTION = { And make it easier for me to try to figure out how to make this bit of code in tools/toolchain/compiler.py work:
(my python Kung Fu is the worst) But this code works on Power7 running a lpar kernel with a /proc/cpuinfo that looks like the below. So far I have (gcc only): $ find /software/easybuild/modulefiles/ -type f | grep -c . Anyway... I strongly endorse this update (since I'm under a lot of pressure to get EasyBuild working on P7 and am hoping to at least get GCC stable/happy while trying to create ibmxl.py and friends for the IBM compilers/etc.). |
except ValueError: | ||
pass | ||
|
||
raise SystemToolsException('Can not determine number of cores on this system') | ||
if core_cnt is None: | ||
raise SystemToolsException('Can not determine number of cores on this system') |
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.
and POWER
?
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.
nothing specific for POWER here, we only need to differentiate between Linux and Darwin
Refer to this link for build results (access rights to CI server needed): |
model = out.strip() | ||
_log.debug("Determined CPU model on Darwin using cmd '%s': %s" % (cmd, model)) | ||
|
||
if model is None: |
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.
nothing for power?
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.
sure, see the comment right above where model_regex
is defined
Refer to this link for build results (access rights to CI server needed): |
…n True for specified path
Refer to this link for build results (access rights to CI server needed): |
from vsc.utils.affinity import sched_getaffinity | ||
except ImportError: | ||
pass | ||
from vsc.utils.affinity import sched_getaffinity |
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.
oooh, this uses ctypes and assumes stuff about glibc. careful with this one on non-linux.
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.
only used on Linux
@boegel looks ok, regexps are unnecessarily complicated though |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
remarks fixed, unit tests are happy, so (finally!) good to go in, in time for EasyBuild v2.0 thanks to @ocaisa, I've been able to confirm that this basically passes all tests on Linux/POWER too (there's a small issue w.r.t. a hanging test (or test setup) in thanks for the thorough review, @wpoely86 and @stdweird, and @JackPerdue for the feedback (there's a dedicated |
enhance systemtools.py to also support POWER systems
fixes issues reported in easybuilders/easybuild#32 by @chwilk