Bug 1134835 - Review Request: kissplice - Detection of various kinds of polymorphism in RNA-seq data
Review Request: kissplice - Detection of various kinds of polymorphism in RNA...
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-NEEDSPONSOR FE-SCITECH
  Show dependency treegraph
 
Reported: 2014-08-28 06:11 EDT by David Parsons
Modified: 2016-05-31 06:12 EDT (History)
4 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description David Parsons 2014-08-28 06:11:24 EDT
Spec URL: http://parsons.eu/tmp/kissplice.spec
SRPM URL: http://parsons.eu/tmp/kissplice-2.2.1-1.fc20.src.rpm
Description: KisSplice is a piece of software that enables to analyze RNA-seq data with or without a reference genome.
It is an exact local transcriptome assembler that allows to identify SNPs, indels and alternative splicing events.
Fedora Account System Username: dparsons

This is my first package, I therefore need a sponsor.

rpmlint complains about 3 spelling errors which are false positives.
It also gives a W: non-standard-group but since Fedora no longer uses groups and this group is used by other distros, I think this is OK
Finally, it gives a W: incoherent-version-in-changelog. I am not sure if I should add the .fc20 postfix (but I think not)

koji builds for f20 and f21:
http://koji.fedoraproject.org/koji/taskinfo?taskID=7473394
http://koji.fedoraproject.org/koji/taskinfo?taskID=7473422
Comment 1 Pierre-YvesChibon 2014-08-29 05:08:56 EDT
In your changelog, you should adjust:
`* Wed Aug 27 2014 David Parsons
<david.parsons@inria.fr> 2.2.1-1`
so that it's one line

The Group tag (Group: Sciences/Biology) are no longer used in Fedora.

The line in %files: %defattr(-,root,root,-) can be dropper, it's no longer used in Fedora.

Similarly, in Fedora the ``rm -rf %{buildroot}`` can be removed at the top of the %install section

That being said, if you intend to package this application for EPEL as well, this last comment does not apply (You'll need rm -rf %{buildroot} at the top of %install)
Comment 2 David Parsons 2014-08-29 05:44:00 EDT
Thank you Pierre-Yves

I have applied your suggestions except for the last once since I indeed intend to package for EPEL as well.

Here's the new SPEC and SRPM files :
http://parsons.eu/tmp/kissplice.spec
http://parsons.eu/tmp/kissplice-2.2.1-2.fc20.src.rpm

I've incremented the release number though I'm not sure I should have...
Thanks again
Comment 3 Pierre-YvesChibon 2014-08-29 05:55:01 EDT
Incrementing the release number was good, replacing the entry for 2.2.1-1 in the changelog not :)

The changelog contains a description of the changes that occurred to the spec file across versions and releases.

So at the moment it should look something like:
``
* Fri Aug 29 2014 David Parsons <david.parsons@inria.fr> 2.2.1-2
- Drop the %%defattr(-,root,root,-) line from %%files
- Drop the Group tag
- Fix changelog format

* Wed Aug 27 2014 David Parsons <david.parsons@inria.fr> 2.2.1-1
- Initial version
``

It's important to have 1 blank line between two changelog entries (some tools rely on that).

During the review process you should bump the release and adjust the changelog for every changes that the reviewer ask you to do. This way when you import your spec file in Fedora it will contain the history of the changes made during the review.
Comment 4 David Parsons 2014-08-29 06:17:42 EDT
Yes, I should have seen that. Well, here come v3 then...

http://parsons.eu/tmp/kissplice.spec
http://parsons.eu/tmp/kissplice-2.2.1-3.fc20.src.rpm
Comment 5 Christopher Meng 2014-08-29 06:40:55 EDT
V3 is not enough, v8 might be good ;)

Something unsatisfied:

1. Drop BR gcc-c++.

2. Drop rm -rf %{buildroot}

3. This is superfluous:

%{_exec_prefix}/lib/kissplice/*

This package is not noarch packages(arch independant), we put libs on X86_64 at %{_libdir} aka /usr/lib64.

If your package installs programs to /usr/lib, I think it's wrong, at least should be /usr/bin.

I haven't taken deep look on those files(only from koji), can you tell us what are these?
Comment 6 Pierre-YvesChibon 2014-08-29 06:53:54 EDT
(In reply to Christopher Meng from comment #5)
> 2. Drop rm -rf %{buildroot}

Maybe you could pay attention to the discussion occurring on a ticket before commenting?
Comment 7 Christopher Meng 2014-08-29 07:01:48 EDT
(In reply to Pierre-YvesChibon from comment #6)
> (In reply to Christopher Meng from comment #5)
> > 2. Drop rm -rf %{buildroot}
> 
> Maybe you could pay attention to the discussion occurring on a ticket before
> commenting?

Why do you think that rm -rf %buildroot is needed on EPEL? EPEL6 doesn't need that, the latest EPEL5 allegedly doesn't need it as well.
Comment 8 Pierre-YvesChibon 2014-08-29 07:06:23 EDT
(In reply to Christopher Meng from comment #7)
> (In reply to Pierre-YvesChibon from comment #6)
> > (In reply to Christopher Meng from comment #5)
> > > 2. Drop rm -rf %{buildroot}
> > 
> > Maybe you could pay attention to the discussion occurring on a ticket before
> > commenting?
> 
> Why do you think that rm -rf %buildroot is needed on EPEL? EPEL6 doesn't
> need that, the latest EPEL5 allegedly doesn't need it as well.

So you may want to check the guidelines: http://fedoraproject.org/wiki/EPEL:Packaging#Prepping_BuildRoot_For_.25install
Comment 9 Pierre-YvesChibon 2014-08-29 07:07:26 EDT
@David you probably want to check that link as well, and if you are planning on packaging this application for epel5, you probably want to look at the next section ;-)
Comment 10 David Parsons 2014-08-29 08:22:27 EDT
Hi Christopher and thank you for adding your comments.

I had seen that point 1. (BR gcc-c++) wasn't needed but I thought it was harmless so I had left it in. I will remove it.

For point 2. I'm wondering whether it is useful to target EPEL5 as well as EPEL6. However, if EPEL5 is still being used, I suppose it would be best to support it and hence keep the "rm -rf %{buildroot}" statement in the %install section.

As for point 3. I had read this: http://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir (files in lib are indeed "executable programs that are designed primarily to be run by other programs rather than by users") but hadn't actually grasped that %{_libdir} would be /usr/lib64 on X86_64.
I understand I have to change the install dir of these files. However I do not know how to do this provided that upstream do not offer the possibility to choose the lib dir. These files are consistently placed in $prefix/lib/kissplice/ Can you help me with this ?

Thanks,
David
Comment 11 Ralf Corsepius 2014-08-29 09:02:18 EDT
(In reply to David Parsons from comment #10)

> For point 2. I'm wondering whether it is useful to target EPEL5 as well as
> EPEL6.
It's up to your personal perference.

> However, if EPEL5 is still being used, I suppose it would be best to
> support it and hence keep the "rm -rf %{buildroot}" statement in the
> %install section.
IMO, it's much easier, cleaner and less error-prone to remove all legacy rpm stuff from Fedora rpms and re-add it if necessary on the corresponding branches in git.
Comment 12 Christopher Meng 2014-08-29 09:06:51 EDT
Hi David,

Ralf's comments are right, IMO less legacy RPM stuffs in SPEC will make it cleaner. Also one more important, if you don't use EPEL, don't maintain packages on that branch.

I can help you by proposing a patch later, you should contact upstream first to get their feedback.

Finally, you need a sponsor first, *sigh... Hope you can get sponsored soon.
Comment 13 David Parsons 2014-08-29 10:04:15 EDT
OK I will remove "rm -rf %{buildroot}" from %install and add it back into the EPEL5 package when needed.

As for the %{_prefix}/lib vs %{_libdir} (or %{_libexecdir}), I'm a former member of the development team of kissplice so I could try and adapt the build process.
These files would however need to remain in %{_prefix}/lib in debian style distributions and I'm not sure how to achieve this in a clean way with CMake... Also, I'm pretty sure there is a simple way to tell rpm to put in %{_libexecdir} what it will find in %{_prefix}/lib, is there not ?

Thanks,
David
Comment 14 Pierre-YvesChibon 2014-08-29 11:59:52 EDT
(In reply to Ralf Corsepius from comment #11)
> (In reply to David Parsons from comment #10)
> > However, if EPEL5 is still being used, I suppose it would be best to
> > support it and hence keep the "rm -rf %{buildroot}" statement in the
> > %install section.
> IMO, it's much easier, cleaner and less error-prone to remove all legacy rpm
> stuff from Fedora rpms and re-add it if necessary on the corresponding
> branches in git.

I only partly agree with this, I prefer to review the most complete spec file even if things are later cleaned in git. So if David wants to support EL5, basically I prefer to review the EL5 spec since it will be Fedora compatible anyway...
Comment 15 Pierre-YvesChibon 2014-08-29 12:00:32 EDT
> Finally, you need a sponsor first, *sigh... Hope you can get sponsored soon.

Unless I'm mistaking, I already see two sponsors on that review :)
Comment 16 Pierre-YvesChibon 2014-08-29 12:02:50 EDT
(In reply to David Parsons from comment #10)
> As for point 3. I had read this:
> http://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir (files in lib
> are indeed "executable programs that are designed primarily to be run by
> other programs rather than by users") but hadn't actually grasped that
> %{_libdir} would be /usr/lib64 on X86_64.

If the files in %{_libexecdir} are meant to be executed but not by the user, then my thoughts are that this folder is appropriate and I don't see the need to move them to %{_libdir}.
If the files are not meant to be executed, then we should look into putting them in %{_libdir}
Comment 17 Ralf Corsepius 2014-08-29 12:52:41 EDT
(In reply to Pierre-YvesChibon from comment #14)

> I only partly agree with this, I prefer to review the most complete spec
> file even if things are later cleaned in git.
I've seen way too many rotten and overly complex specs, to support this thought.
Comment 18 David Parsons 2014-08-29 12:56:17 EDT
(In reply to Pierre-YvesChibon from comment #16)
> (In reply to David Parsons from comment #10)
> > As for point 3. I had read this:
> > http://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir (files in lib
> > are indeed "executable programs that are designed primarily to be run by
> > other programs rather than by users") but hadn't actually grasped that
> > %{_libdir} would be /usr/lib64 on X86_64.
> 
> If the files in %{_libexecdir} are meant to be executed but not by the user,
> then my thoughts are that this folder is appropriate and I don't see the
> need to move them to %{_libdir}.
> If the files are not meant to be executed, then we should look into putting
> them in %{_libdir}

The files are indeed meant to be executed by a program (not directly by the user). However, they are not in %{_libexecdir} but in %{_prefix}/lib. This conforms to FHS and debian packaging guidelines but apparently not quite to fedora's...
Would it be as simple as adding "mv %{_prefix}/lib %{_libexecdir}" somewhere?
Comment 19 Christopher Meng 2014-08-30 02:52:39 EDT
(In reply to David Parsons from comment #18)
> (In reply to Pierre-YvesChibon from comment #16)
> > (In reply to David Parsons from comment #10)
> > > As for point 3. I had read this:
> > > http://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir (files in lib
> > > are indeed "executable programs that are designed primarily to be run by
> > > other programs rather than by users") but hadn't actually grasped that
> > > %{_libdir} would be /usr/lib64 on X86_64.
> > 
> > If the files in %{_libexecdir} are meant to be executed but not by the user,
> > then my thoughts are that this folder is appropriate and I don't see the
> > need to move them to %{_libdir}.
> > If the files are not meant to be executed, then we should look into putting
> > them in %{_libdir}
> 
> The files are indeed meant to be executed by a program (not directly by the
> user). However, they are not in %{_libexecdir} but in %{_prefix}/lib. This
> conforms to FHS and debian packaging guidelines but apparently not quite to
> fedora's...

Each project has its own way to handle files. And I don't agree with Debian all the time.

> Would it be as simple as adding "mv %{_prefix}/lib %{_libexecdir}" somewhere?

You can try and checkout to see if it works as its default behavior. Putting something in /usr/lib directly is the fault from upstream as they don't eithor respect the libdir parameter which could be defined during CMake or take into account of /lib64 on some OS, e.g Fedora, RHEL.
Comment 20 David Parsons 2014-09-03 06:04:39 EDT
Hi,

I've submitted a patch to upstream that fixes the build issue. I was even lucky enough to come across the team leader yesterday, they will talk about it at their next meeting on friday. I'm confident it will be included in the next release.

In the meantime I've tried the solution I mentionned above (which is actually "mv %{buildroot}/%{_prefix}/lib %{buildroot}/%{_libexecdir}") and it looks like it's working just fine.

Here's the new files 
http://parsons.eu/tmp/kissplice.spec
http://parsons.eu/tmp/kissplice-2.2.1-4.fc20.src.rpm

Thanks,
David

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