-
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
enhance Amber easyblock for AmberTools #2618
base: develop
Are you sure you want to change the base?
Conversation
Co-authored-by: Sam Moors <smoors@users.noreply.github.com>
easybuild/easyblocks/a/amber.py
Outdated
# Run the tests located in the build directory, only for Amber. Tests fail when building only AmberTools | ||
if self.cfg['runtest'] and self.name == 'Amber': |
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.
do all tests fail with AmberTools, or only some?
if not all tests fail, I think it's still useful to run them in combination with EB option --ignore-test-failure
.
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 did not try to untangle the tests, but not all executables from Amber get created, and the tests assume that they are there.
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.
See #2604 (and easybuilders/easybuild-easyconfigs#14028) - it is trying to run the Amber test suite and not the AmberTools test suite. There were still some failures with the AmberTool test suite and I've not had the chance to investigate further.
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, we're doing the runtest part wrong. I think I have the correct stuff in my test-.eb but I got sidetracked and haven't had time to go back and revisit.
It should not matter if we build Amber or AmberTools the test part (CMake and old style) is written so that it only tests what is actually built, if we call it correctly. I checked with the devs on that.
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.
@akesandgren did you hear back from the Amber devs yet?
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.
They basically said what I said above, The CMake part should do the right thing when asked to run make test.
But I haven't had time to revisit this on my side...
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 still fails as of today. Can't build AmberTools without disabling tests.
PR #2720 addresses some of the same points as this PR, but not all of them. In particular, it does not disable tests for AmberTools, and so the build of Ambertools fails |
PR #2781 makes sure the tests are run the correct way and should eliminate that problem. |
@mboisson Can you sync this PR with current |
Done |
The NetCDF-Fortran problem is solved with AmberTools-20_cmake-locate-netcdf.patch but doing it directly in the easyblock might be beneficial. Thoughts on the above two @mboisson ? (And the testing is finally solved with the latest easyblock+easyconfigs+patches, apart from a few remaining test failures) |
Mmm, I don't know. Patches have to be maintained from version to version. If there are configure flags that allow us to do the same work, I would rather not maintain patches but use those options. |
This PR allows a few tings
usempi
(to get serial compilers for serial code) and enable MPI through the CMake option, it fails to build.4) Disable tests when building AmberTools, as not all the binaries are included.