Bug 841266 - Review Request: plink - whole genome association analysis toolset
Review Request: plink - whole genome association analysis toolset
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-SCITECH
  Show dependency treegraph
 
Reported: 2012-07-18 10:16 EDT by Ray Pete
Modified: 2013-10-19 10:42 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-09-01 15:01:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


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

  None (edit)
Description Ray Pete 2012-07-18 10:16:33 EDT
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 10:20:56 EDT
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 16:21:01 EDT
URL tag has an extra http
Comment 3 Ray Pete 2012-07-21 18:05:22 EDT
Thanks Rasmus fixed the duplicate http:// tag
Comment 4 Dmitrij S. Kryzhevich 2012-07-30 02:31:12 EDT
If you need to be sponsored, please, mark this in Blocks.
Comment 5 Volker Fröhlich 2012-10-14 14:59:32 EDT
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 12:16:20 EST
Are you still interested in this package?
Comment 7 Ray Pete 2012-11-26 14:04:02 EST
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.

Cheers,

Ray
Comment 8 Ray Pete 2012-11-27 21:28:46 EST
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

http://pngu.mgh.harvard.edu/~purcell/plink/rfunc.shtml


Cheers,

Ray
Comment 9 Ray Pete 2012-11-27 21:30:36 EST
umm.. typo..

s/output R code/input R plugin code/
Comment 10 Volker Fröhlich 2012-11-28 21:05:40 EST
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-28 21:40:01 EST
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 15:50:58 EST
made configure script, added plink man page, took out gplink.jar file

http://raymondpete.fedorapeople.org/plink-1.07-2.fc17.src.rpm
http://raymondpete.fedorapeople.org/plink.spec
Comment 13 Volker Fröhlich 2013-04-26 12:46:45 EDT
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 19:10:27 EDT
Sorry Volker missed your emails.
updates to follow with the corrected spec.
Comment 16 Jason Tibbitts 2013-06-13 22:45:37 EDT
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 
   toolset
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-19 21:06:13 EDT
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-19 23:01:34 EDT
http://raymondpete.fedorapeople.org/plink.spec
http://raymondpete.fedorapeople.org/plink-1.07-4.fc19.src.rpm

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 15:45:20 EDT
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 10:07:32 EDT
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.

http://raymondpete.fedorapeople.org/plink-1.07-5.fc19.src.rpm
http://raymondpete.fedorapeople.org/plink.spec

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