Bug 841266 - Review Request: plink - whole genome association analysis toolset
Summary: Review Request: plink - whole genome association analysis toolset
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2012-07-18 14:16 UTC by Ray Pete
Modified: 2017-02-07 04:33 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2013-09-01 19:01:25 UTC
Type: ---

Attachments (Terms of Use)
koji build log (17.08 KB, text/plain)
2012-07-18 14:20 UTC, Ray Pete
no flags Details

Description Ray Pete 2012-07-18 14:16:33 UTC
Spec URL: www.iski.tv/plink.spec
SRPM URL: www.iski.tv/plink-1.07-1.fc17.src.rpm
Description: whole genome association analysis toolset, designed to perform a range of basic, large-scale analyses in a computationally efficient manner. 

Fedora Account System Username: raymondpete

Comment 1 Ray Pete 2012-07-18 14:20:56 UTC
Created attachment 598901 [details]
koji build log

built on i686 and x86_64 fine
source needed to be modified to build with gcc-4.7
Variable duplication errors
Also needed to add auto detection on 32 versus 64 during make

Comment 2 Rasmus Ory Nielsen 2012-07-21 20:21:01 UTC
URL tag has an extra http

Comment 3 Ray Pete 2012-07-21 22:05:22 UTC
Thanks Rasmus fixed the duplicate http:// tag

Comment 4 Dmitrij S. Kryzhevich 2012-07-30 06:31:12 UTC
If you need to be sponsored, please, mark this in Blocks.

Comment 5 Volker Fröhlich 2012-10-14 18:59:32 UTC
The license must be stated as "GPLv2". https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing

The build does not respect Fedora's optflags. http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

What is it about the R plug-in?

There are a couple of compiler warnings that could be dealt with.

Please use the name and version macro in the Source0 line.

Drop the two Requires. http://fedoraproject.org/wiki/Packaging:Guidelines#Requires

install -m 0755 plink %{buildroot}%{_sbindir}/plink should be fine.

Why is that installed to sbindir?

Add COPYING.txt and README via the doc macro.

I noticed a jar file in the tarball. What is it?

Comment 6 Volker Fröhlich 2012-11-26 17:16:20 UTC
Are you still interested in this package?

Comment 7 Ray Pete 2012-11-26 19:04:02 UTC
Hi Volker,

Indeed I have updated the spec per your requests above. 
I'll submit the new srpm to koji and update the ticket when all is completed.

Regarding the .jar file. This is a gui application, which does not seem to include src.
I also dealt with a few of the warnings mentioned above, but this is not included on the website source yet.



Comment 8 Ray Pete 2012-11-28 02:28:46 UTC
Hi Volker,

So updated spec and srpm linked

Spec URL: www.iski.tv/plink.spec
SRPM URL: www.iski.tv/plink-1.07-1.fc17.src.rpm

I believe things are in better shape now.
Please let me know if you notice something missed? And tx for your help reviewing

FYI, the R plugin is merely an option in the plink binary to output R code




Comment 9 Ray Pete 2012-11-28 02:30:36 UTC
umm.. typo..

s/output R code/input R plugin code/

Comment 10 Volker Fröhlich 2012-11-29 02:05:40 UTC
Whenever you publish a new version of your files in the future, please bump the release number. That makes work easier for reviewers.

Comment 11 Ray Pete 2012-11-29 02:40:01 UTC
Spec URL: http://www.iski.tv/plink.spec
SRPM URL: http://www.iski.tv/plink-1.07-1.fc17.src.rpm

Comment 12 Ray Pete 2012-12-08 20:50:58 UTC
made configure script, added plink man page, took out gplink.jar file


Comment 13 Volker Fröhlich 2013-04-26 16:46:45 UTC
You seem to have ignored a few of my previous commments. I'm repeating some of them here.

Use the %{_mandir} macro.

Why is the binary in /usr/sbin? http://www.pathname.com/fhs/pub/fhs-2.3.html#USRSBINNONESSENTIALSTANDARDSYSTEMBI

"mkdir -p %{buildroot}%{_mandir}/man1" is enough. You don't need 4 mkdir invocations.

Why are you not using "make %{?_smp_mflags}"?

Use the version macro in Source0.

Comment 14 Ray Pete 2013-06-10 23:10:27 UTC
Sorry Volker missed your emails.
updates to follow with the corrected spec.

Comment 16 Jason Tibbitts 2013-06-14 02:45:37 UTC
So I downloaded and built this; anyone who is still interested after nearly a year certainly deserves a review.  However, I immediately noticed a few issues:

The package doesn't build with the mandatory compiler flags.  If that's a regular autoconf-generated script (and it sure looks like it is), why not use the usual %configure macro to call it, so that everything gets set and passed correctly?

Did you look at the rpmlint output at all?  I see a bunch of stuff which is trivially fixable and some other stuff I don't understand at all.

  plink-debuginfo.x86_64: E: debuginfo-without-sources
This comes from not building with the mandatory compiler flags.  (-g isn't there, so no debug info was compiled in.)

  plink.x86_64: W: manual-page-warning /usr/share/man/man1/plink.1.gz 28: 
   warning: numeric expression expected (got a tab character)
A whole bunch of these which I don't understand.  Generally you should not compress manpages; rpm will do it automatically, and will use the proper compression system if we decide to switch from gzip in the future.

  plink.src: W: summary-not-capitalized C whole genome association analysis 
Should capitalize the summary.

  plink.src:28: W: configure-without-libdir-spec
The configure script should be called via %configure if it is autoconf-generated.

  plink.src:10: W: mixed-use-of-spaces-and-tabs (spaces: line 10, tab: line 1)
Please be consistent with indentation.

  plink.src: W: file-size-mismatch plink-1.07-src.zip = 692437, 
   http://pngu.mgh.harvard.edu/~purcell/plink/dist/plink-1.07-src.zip = 2257297
So, what happened here?  The file in the src.rpm is pretty much completely different than what's at the upstream web site.

Comment 17 Ray Pete 2013-06-20 01:06:13 UTC
Thanks for the poke.
indeed I failed to run rpmlint. reviewing now
With respect to the src mismatch. The zip original contains a jar file with no source, which is merely a gui. I removed that due to source missing, and also created the autoconf stuff since it only had a static Makefile

Comment 18 Ray Pete 2013-06-20 03:01:34 UTC

rpmlint comes back clean with the exception of the aforementioned upstream size mismatch due to the jar file.

Comment 19 Jason Tibbitts 2013-06-20 19:45:20 UTC
There are only very limited situations where it is permissible to modify the upstream source archive.  Basically when we cannot distribute some part of it due to legal reasons.

Please either detail the legal reasons why we cannot redistribute the upstream source archive as is, or fix this package to use the archive unmodified.  You can of course remove the unneeded jar file in %prep, and I suppose you could add the autoconf stuff there as well though generally we frown on such things.  Could you detail the specific reasons why the upstream makefile is not useful, and cannot be minimally fixed with basic patching?

Comment 20 Ray Pete 2013-06-23 14:07:32 UTC
Tx Jason.
So based on your last comments. You will see the exact src.zip file now used with patch files applied to fix various errors, which were picked up with the newer GCC.
rpmlint is clean. Appreciate the guidance while I work through the SOP. Koji make was clean.


Note You need to log in before you can comment on or make changes to this bug.