-
Notifications
You must be signed in to change notification settings - Fork 307
update Siesta easyblock for v3.2 to 4.1-b3 #1510
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
update Siesta easyblock for v3.2 to 4.1-b3 #1510
Conversation
Some things where missing from 4.0.x versions. Also sorted the lists in alphabetical order, and added some missing shell-scripts.
easybuild/easyblocks/s/siesta.py
Outdated
| # make sure Siesta was indeed built with support for running in parallel | ||
| custom_commands.append("echo 'SystemName test' | mpirun -np 2 siesta 2>/dev/null | grep PARALLEL") | ||
| # The "cd to builddir" is required to not contaminate the install dir with cruft from running siesta | ||
| custom_commands.append("cd %s && echo 'SystemName test' | mpirun -np 2 siesta 2>/dev/null | grep PARALLEL" % self.builddir) |
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.
line too long (135 > 120 characters)
easybuild/easyblocks/s/siesta.py
Outdated
| # remove clean at the end of default target | ||
| if self.version == '4.0.1' or self.version == '4.1-b3': | ||
| # And yes, they are re-introducing this bug. | ||
| if LooseVersion(self.version) >= LooseVersion('4.0') and LooseVersion(self.version) < LooseVersion('4.0.2') or LooseVersion(self.version) == LooseVersion('4.1-b3'): |
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.
line too long (176 > 120 characters)
easybuild/easyblocks/s/siesta.py
Outdated
| (r"^(FPPFLAGS\s*=.*)$", r"\1 -DCDF $(NETCDF_INCLUDE)"), | ||
| ]) | ||
| regex_newlines.append((r"^(COMP_LIBS\s*=.*)$", r"\1\nNETCDF_LIBS = -lnetcdff")) | ||
| regex_newlines.append((r"^(COMP_LIBS\s*=.*)$", r"\1\nNETCDF_LIBS = -lnetcdff\nNETCDF_INCLUDE = -I%s/include" % netcdff_loc)) |
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.
line too long (140 > 120 characters)
easybuild/easyblocks/s/siesta.py
Outdated
| # And yes, they are re-introducing this bug. | ||
| if ((LooseVersion(self.version) >= LooseVersion('4.0') | ||
| and LooseVersion(self.version) < LooseVersion('4.0.2')) | ||
| or LooseVersion(self.version) == LooseVersion('4.1-b3')): |
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.
visually indented line with same indent as next logical line
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 what does it really want it to be ???
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.
Should be left one column, aligned with the ( on line 231?
This is messy though... How about:
loose_ver = LooseVersion(self.version)
is_ver40 = loose_ver >= LooseVersion('4.0') and loose_ver < LooseVersion('4.0.2')
if is_ver40 or loose_ver == LooseVersion('4.1-b3'):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 obviously the loose_ver definition can be higher up, so you can use it throughout the whole method and avoid using LooseVersion(self.version) all over the place...
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.
Just what i was about to ask :-)
…ded. Also make complex if-stmt more readable.
boegel
left a 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.
lgtm
|
Changes make sense, tested with all existing Thanks @akesandgren! |
Fixes some lingering bugs for various versions, and updates lists of Util programs for versions 3.2 to 4.1-b3