{bio}[foss/2018a] add MEGAHIT-1.1.2 #5748
Conversation
|
A prior Travis check bombed with There are potentially two things wrong here. First, a typo--actually Python-2.7.14 is being used. The other might be a complaint about using the Trying it out after fixing the first only, it still fails, so presumably the |
|
Test report by @verdurin |
|
Test report by @boegel |
|
Test report by @boegel |
|
@tutufan - normally you would use Does that clarify? |
|
@verdurin Ah, derp. I get it. The calculated suffix is just coming from name, version, and versionsuffix, and the easyconfig filename has to match. So, that mistake notwithstanding, I could have gone either way here. In this case, the package absolutely depends on Python 2, but probably doesn't care that much about the specific version. Should this be changed? |
|
@tutufan The easyconfig filename must be |
| easyblock = 'MakeCp' | ||
|
|
||
| name = 'MEGAHIT' | ||
| version = '1.1.2' |
There was a problem hiding this comment.
If Python is a dependency, we usually make that clear via the versionsuffix (since Python is a very common dependency)
versionsuffix = '-Python-%(pyver)s`Note that including this implies that you need to rename the easyconfig file to end with -Python-2.7.14.eb...
| checksums = ['d0d3965dd49c6fdaea958ef66146cb6b30b7d51acbbfe94194c437f15a424cb5'] | ||
|
|
||
| # This version is CPU-only. It's also possible to compile this with | ||
| # CUDA, but that isn't working yet (and requires libcuda.so!). |
There was a problem hiding this comment.
@tutufan Hmm, not sure if it's worth keeping this comment in?
Not having CUDA as dependency, and the gpu=0 make it fairly clear this is a non-GPU build?
| ] | ||
|
|
||
| # for GPU, 'use_gpu=1 abi=1' at least, plus CUDA dep above | ||
| buildopts = 'version=v%(version)s verbose=1 use_gpu=0' |
There was a problem hiding this comment.
This looks a bit strange, what would happen if you don't include the version=v%(version)s part?
Maybe it's worth clarifying that with a comment above this line?
There was a problem hiding this comment.
In the Makefile, version will be set to git describe --tag. I'm not sure that even works for release tarballs. Even if it does, though, it potentially fails badly if the commit that has a release tag also has other tags. Since we know the version (since we're using it to pull the same tag from github), it seems safest to specify it explicitly. Will add a comment.
| runtest = 'test' | ||
|
|
||
| sanity_check_paths = { | ||
| 'files': ['bin/%(namelower)s'], |
| '%(namelower)s_asm_core', | ||
| '%(namelower)s_sdbg_build', | ||
| '%(namelower)s_toolkit'], | ||
| 'bin'), |
There was a problem hiding this comment.
minor style suggestion:
files_to_copy = [
(['%(namelower)s', '%(namelower)s_asm_core', '%(namelower)s_sdbg_build', '%(namelower)s_toolkit'], 'bin'),
'LICENSE',
'README.md',
]There was a problem hiding this comment.
I considered this, but had the idea that multi-element lists were to be one-per-line. (In fact, you requested that in #5698.) Is there an easy rule-of-thumb to remember?
There was a problem hiding this comment.
The format I suggested makes it clearer that there's basically 3 things being copied: binaries, LICENSE & README.
There's no clear rule for this, it's mostly intuition (and potential a bike-shedding opportunity).
New module. This version is cpu-only. It's also possible to compile this with CUDA, but it seems useful to have two separate versions. (Still working on getting a CUDA version to compile.)
|
@boegel This latest version addresses your comments, I believe. Some of the comments were breadcrumbs to me or to whomever follows, pointing out a bit of how to proceed for a CUDA built. Anyway, those are mostly zapped. I tried the "all on one line" approach in |
|
@tutufan - the simple workaround here would be to remove one of the sanity check entries. It's acceptable not to check for every single binary, as long as a meaningful number are covered. |
|
@verdurin That would be my thought as well, but @boegel requested above that all binaries be checked. My personal thought would be that list formatting is mostly a matter of taste, though I'd prefer respecting PEP8 with respect to the 80-column limit. I don't really care, though, as long as the rule is spelled out in enough detail that it's possible to hit the target without iterating. |
|
Updated with @verdurin 's suggestion, in hopes of getting this in. |
|
Kicking Travis, which failed on one of eight runs due to a timeout/etc during download of the source. (It would be nice if the downloader would try a bit harder before giving up.) |
|
@tutufan If the line becomes too long, just wrap to the next line. Dropping one if fine too, since the binaries are being copied it actually can't go wrong undetected in this case, either the copying works and the binaries are there, or the copying fails and then it never gets to the sanity check (copying won't fail silently). Looks good as is now, thanks for your persistence. We try to be consistent (in places where Travis isn't on our behalf), but we're only human. ;-) |
|
Test report by @boegel |
|
Test report by @boegel |
|
Thanks @tutufan! |
New module. This version is cpu-only. It's also possible to compile
this with CUDA, but it seems useful to have two separate
versions. (Still working on getting a CUDA version to compile.)