-
Notifications
You must be signed in to change notification settings - Fork 284
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 sanity tests for OpenFOAM, add logging #1205
Conversation
problem with the tests should be fixed now that easybuilders/easybuild-framework#2258 is merged; I have retriggered the tests... |
- this allows redirection of the Allwmake output to a log file so that it is possible to debug any errors with the build process and have a record of the configured build options.
- The 'wdot' utility has been deprecated in favour of 'postProcess', so don't wdot for the sanity check. - For scripts, only 'paraFoam' is relatively safe to check. The foamExec script may not last since it uses some outdated FOAM_INST_DIR semantics.
13b9ad1
to
4fddf72
Compare
Hello @easybuilders/easybuild-easyblocks-maintainers - please review |
@@ -281,6 +281,9 @@ def build_step(self): | |||
"Cleaning .*", | |||
] | |||
run_cmd_qa(cmd_tmpl % 'Allwmake.firstInstall', qa, no_qa=noqa, log_all=True, simple=True) | |||
elif LooseVersion(self.version) >= LooseVersion('1600'): |
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.
Hmm, this version check looks wrong? OpenFOAM versions are like 2.3.1
, 3.0.1
, etc.
So, the condition here would never be met?
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.
Actually we now have versions following YYMM, so 1606, 1612, 1706 thus far with 1706 having been released at the end of June-2017.
http://openfoam.com/releases/openfoam-v1706/
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.
Ah, OK, I overlooked that, my apologies.
So, the -log
is only supported since the 1600
version?
I'd also suggest to cleaning this up a bit, feels too copy-paste as it is now:
else:
cmd = 'Allwmake'
if LooseVersion(self.version) >= LooseVersion('1600'):
# instruct to redirect output of Allwmake to a log file, for easier debugging & future reference
cmd += ' -log'
run_cmd(cmd_tmpl % cmd, log_all=True, simple=True, log_output=True)
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.
It was added in 1606. How would you clean this up? (Too much of the package looks like cut-n-paste already).
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.
Then you should modify the version check to >= LooseVersion('1606')
?
See code snippet above for a suggestion on how to avoid copy-pasting.
[os.path.join(toolsdir, x) for x in ["deformedGeom", "engineSwirl", "modifyMesh", | ||
"refineMesh", "wdot"]] | ||
[os.path.join(toolsdir, x) for x in ["checkMesh", "deformedGeom", "engineSwirl", | ||
"modifyMesh", "refineMesh"]] |
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 these changes to the sanity check, they are unrelated to the -log
change?
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.
Among other things, the wdot
utility has been deprecated. There is a shell script placeholder (in a different location) that now only produces a message about it being redundant. This means that the previous sanity checks will never work.
Since the comments in the sanity check claim to have some arbitrarily chosen applications, I simply replaced wdot
with checkMesh
which should continue to exist for a while.
These sanity checks could actually be further reduced to blockMesh
and checkMesh
and be done with it.
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.
OK, thanks for clarifying the removal of wdot
, makes sense.
I'd rather keep the others around next to blockMesh
& checkMesh
, since they may help in catching problems with the build, unless you have a good reason for getting rid of those too?
What about the removal of foamExec
, has that been deprecated/removed in recent versions too?
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, no harm keeping around others. If foamExec hasn't been removed, then I agree that it makes a bad sanity test. Probably the only really safe script to check is paraFoam, the others may be too much of a moving target.
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.
Hmm, not sure what you are saying w.r.t. foamExec... Was it removed in recent OpenFOAM versions?
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 original design of foamExec was to help starting up with identical paths, versions on various machines when not using a scheduler. While this generally still works, the upgrades to OpenFOAM configuration files means that the foamExec provision for starting an alternative OpenFOAM version and/or an alternative prefix location is quite broken. So I think that it (and foamJob) are in need of an overhaul or removal. In either case, a perhaps slightly dubious future.
@@ -281,6 +281,9 @@ def build_step(self): | |||
"Cleaning .*", | |||
] | |||
run_cmd_qa(cmd_tmpl % 'Allwmake.firstInstall', qa, no_qa=noqa, log_all=True, simple=True) | |||
elif LooseVersion(self.version) >= LooseVersion('1600'): |
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.
Ah, OK, I overlooked that, my apologies.
So, the -log
is only supported since the 1600
version?
I'd also suggest to cleaning this up a bit, feels too copy-paste as it is now:
else:
cmd = 'Allwmake'
if LooseVersion(self.version) >= LooseVersion('1600'):
# instruct to redirect output of Allwmake to a log file, for easier debugging & future reference
cmd += ' -log'
run_cmd(cmd_tmpl % cmd, log_all=True, simple=True, log_output=True)
@olesenm ping on following up on this? It would be nice to get this merged in soon... |
Just a note on the version thingie. Remember that there are versions of openfoam from both openfoam.com and openfoam.org. The openfoam.org versions are still at 5.x which is approx the same as the 17xx version from openfoam.com. Some users want the openform.org version and the easyblock needs to handle both. |
For now, I don't think there's a problem, but that may change in the future if the @olesenm Can we think of a way to discriminate between the If needed, we can consider renaming the |
@boegel : first day back from holidays (thus unresponsive for a period). For spack, we have The configuration files for |
For
where The following snippet should work to catch openfoam-com, but does not provide any distinction between development and release branches (just the major API level).
For For |
it is possible to debug any errors with the build process and have a
record of the configured build options.