Summary: | [NEW PORT] biology/jellyfish: Fast, memory-efficient counting of k-mers in DNA | ||||||
---|---|---|---|---|---|---|---|
Product: | Ports & Packages | Reporter: | Jason W. Bacon <jwb> | ||||
Component: | Individual Port(s) | Assignee: | Dmitry Marakasov <amdmi3> | ||||
Status: | Closed FIXED | ||||||
Severity: | Affects Some People | CC: | rakuco | ||||
Priority: | --- | ||||||
Version: | Latest | ||||||
Hardware: | Any | ||||||
OS: | Any | ||||||
Attachments: |
|
Description
Jason W. Bacon
2016-03-12 18:19:43 UTC
Created attachment 168261 [details]
Shar archive
Some suggestions after a quick review: * You can drop the pkg-config sed call and replace it with USES=pathfix and PATHFIX_MAKEFILEIN=Makefile.am. * The version and architecture checks can be simplified: 1. Use a helper to mark the port as broken: BROKEN_FreeBSD_9_i386= multiple code issues on i386 < 10.0-RELEASE 2. Use MACHINE_CPU to check if the CPU supports SSE: .if empty(MACHINE_CPU:sse) CONFIGURE_ARGS+= --without-sse .endif This also allows you to just include bsd.port.mk. * BUILD_DEPENDS+= should be BUILD_DEPENDS= * Should --without-sse work on amd64? I tried passing it here and the build failed. A commit references this bug: Author: amdmi3 Date: Thu Mar 24 13:53:07 UTC 2016 New revision: 411785 URL: https://svnweb.freebsd.org/changeset/ports/411785 Log: Jellyfish is a tool for fast, memory-efficient counting of k-mers in DNA. A k-mer is a substring of length k, and counting the occurrences of all such substrings is a central step in many analyses of DNA sequence. JELLYFISH can count k-mers quickly by using an efficient encoding of a hash table and by exploiting the "compare-and-swap" CPU instruction to increase parallelism. WWW: http://www.genome.umd.edu/jellyfish.html PR: 207929 Submitted by: bacon4000@gmail.com Changes: head/biology/Makefile head/biology/jellyfish/ head/biology/jellyfish/Makefile head/biology/jellyfish/distinfo head/biology/jellyfish/files/ head/biology/jellyfish/files/patch-include_jellyfish_file__header.hpp head/biology/jellyfish/pkg-descr head/biology/jellyfish/pkg-plist Dmitry, we were in the middle of code review, it would've been nice if you hadn't committed the original patch. I'll submit a maintainer update ASAP. Cheers, Jason (In reply to Raphael Kubo da Costa from comment #4) > Dmitry, we were in the middle of code review, it would've been nice if you > hadn't committed the original patch. The port was not assigned so I take it as mere suggestion, and I don't agree with most of these. > * You can drop the pkg-config sed call and replace it with USES=pathfix and PATHFIX_MAKEFILEIN=Makefile.am. This is a hack. Sed is more straightforward. > * The version and architecture checks can be simplified: > 1. Use a helper to mark the port as broken: > BROKEN_FreeBSD_9_i386= multiple code issues on i386 < 10.0-RELEASE IGNORE is OK if it's not intended to work. But yes, if it's `code issues', BROKEN should be used. > 2. Use MACHINE_CPU to check if the CPU supports SSE: > .if empty(MACHINE_CPU:sse) > CONFIGURE_ARGS+= --without-sse > .endif I don't think you may use MACHINE_CPU in ports. > * BUILD_DEPENDS+= should be BUILD_DEPENDS= Fixed. > I'll submit a maintainer update ASAP. There's no need to hurry, you may submit improvements along with further port updates. BROKEN_FreeBSD_9_i386 is the only one that I'd actually add. (In reply to Dmitry Marakasov from comment #6) > The port was not assigned so I take it as mere suggestion, and I don't agree with most of these. You could have at least said _something_, including asking Jason if he intended to take any of the suggestions or not. It probably wasn't even your intention, but the way this was handled felt like everything I wrote was simply ignored. > > * You can drop the pkg-config sed call and replace it with USES=pathfix and PATHFIX_MAKEFILEIN=Makefile.am. > > This is a hack. Sed is more straightforward. Ues/pathfix.mk exists precisely to avoid the duplication of similar sed calls in the tree. > > * The version and architecture checks can be simplified: > > 1. Use a helper to mark the port as broken: > > BROKEN_FreeBSD_9_i386= multiple code issues on i386 < 10.0-RELEASE > > IGNORE is OK if it's not intended to work. But yes, if it's `code issues', > BROKEN should be used. > > > 2. Use MACHINE_CPU to check if the CPU supports SSE: > > .if empty(MACHINE_CPU:sse) > > CONFIGURE_ARGS+= --without-sse > > .endif > > I don't think you may use MACHINE_CPU in ports. Yes you can. At the moment there are 69 occurrences in the tree and I am not aware of any policies forbidding its use. (In reply to Raphael Kubo da Costa from comment #7) > Ues/pathfix.mk exists precisely to avoid the duplication of similar sed > calls in the tree. It exists to fix Makefile.am, not .in, and aliasing .in as .am looks like a hack and using a tool wrong way. Either pathfix should be fixed to not require PATHFIX_MAKEFILEIN, or plain REINPLACE_CMD should be used in ports. > > I don't think you may use MACHINE_CPU in ports. > > Yes you can. At the moment there are 69 occurrences in the tree and I am not > aware of any policies forbidding its use. The fact that something widely used wrong way is not an argument to use it even more. It's share/mk feature not related to ports, and ports should be as little tied to share/mk features as possible (instead it may lead to inconsistent ports behavior on different major FreeBSD branches and/or dragonfly). I also though it will make a package tied to a features of machine it's built on, but it doesn't seem to be the case. I don't see any proof that it's guaranteed though. Sigh, I can see there's no point in continuing this argument. I guess next time I'll just assign the bug to myself. (In reply to Raphael Kubo da Costa from comment #9) > Sigh, I can see there's no point in continuing this argument. I guess next > time I'll just assign the bug to myself. That's just strange reaction when we're about to resolve either your or main delusion. Oh, and I forgot to mention: Jason, I've removed USES=compiler:openmp because it didn't work (USES=compiler only supports single instance with single argument). Also I haven't found any mention of openmp in the package so I'm not sure it's needed at all. Gentlemen, Disagreements aside, I appreciate all the work you both contribute to the project and the feedback on my ports. I will submit a maintainer update shortly with some of these suggestions. Regards, JB BROKEN appears to be ignored by my Poudriere installation. Is this normal? If I add BROKEN_FreeBSD_10_amd64, I get the expected message when trying to build outside Poudriere, but even BROKEN=message has no effect in Poudriere. It attempts to build anyway. MACHINE_CPU is not going to work in this case. The problem is not whether the underlying CPU supports SSE, but that the #ifdefs in the source code assume SSE means amd64 and vice versa. Regards, JB (In reply to Jason Bacon from comment #13) > BROKEN appears to be ignored by my Poudriere installation. Is this normal? Afaik, `testport' always ignores BROKEN. `bulk' does honor it though. |