-
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
fix default value for custom 'namd_basearch' easyconfig parameter in NAMD easyblock #2123
fix default value for custom 'namd_basearch' easyconfig parameter in NAMD easyblock #2123
Conversation
easybuild/easyblocks/n/namd.py
Outdated
|
||
cuda = get_software_root('CUDA') | ||
if cuda: | ||
basearch = '%s.cuda' % basearch |
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 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.
Fails with:
ERROR: Build of /tmp/eb-zuwacrgi/files_pr11090/n/NAMD/NAMD-2.14-fosscuda-2019b.eb failed
(err: "build failed (first 300 chars): Failed to patch arch/Linux-x86_64.cuda.tcl:
[Errno 2] No such file or directory: 'arch/Linux-x86_64.cuda.tcl' -> 'arch/Linux-x86_64.cuda.tcl.orig.eb'")
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 regarding restoring the namd_basearch
easyconfig parameter
but merging this will actually break the fosscuda
easyconfig (as opposed to cuda being ignored in the basearch), how shall we proceed?
@migueldiascosta Should be fixed now, it simply doesn't make sense to add Tested with both easyconfigs from easybuilders/easybuild-easyconfigs#11090, as well as the ones in easybuilders/easybuild-easyconfigs#11114 |
easybuild/easyblocks/n/namd.py
Outdated
|
||
if self.cfg['namd_basearch'] is None: | ||
|
||
self.log.info("namd_basearch not specified, so determining it based a CPU arch & CUDA dep...") |
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 guess mentioning the CUDA dep here is misleading then
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.
fixed in 32b61e8
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 @boegel! |
(created using
eb --new-pr
)fix for issue raised by @migueldiascosta in #2113