Bug 866325 - Review Request: ugene - genome analysis suite
Review Request: ugene - genome analysis suite
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
: 481717 599952 612333 620005 866321 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-15 01:34 EDT by Yuliya Algaer
Modified: 2013-08-07 08:06 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-12-01 04:53:43 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Yuliya Algaer 2012-10-15 01:34:58 EDT
Spec URL: http://ugene.unipro.ru/downloads/ugene.spec
SRPM URL: http://ugene.unipro.ru/downloads/ugene-1.11.2-1.fc17.src.rpm

Description: 

ReReview is needed for UGENE - complex bioinformatics toolkit based on Qt.

Unipro UGENE is a cross-platform visual environment for DNA and protein
sequence analysis. UGENE integrates the most important bioinformatics
computational algorithms and provides an easy-to-use GUI for performing
complex analysis of the genomic data. One of the main features of UGENE
is a designer for custom bioinformatics workflows.

Fedora Account System Username: yalgaer

See also https://bugzilla.redhat.com/show_bug.cgi?id=481717
Comment 1 Peter Lemenkov 2012-10-15 02:10:39 EDT
I'll review it (and I will sponsor you).
Comment 2 Yuliya Algaer 2012-10-15 03:44:20 EDT
I requested for sponsorship, FAS name: yalgaer. 

Please, approve me as a member of the packager and provenpackager groups.
Comment 3 Peter Lemenkov 2012-10-15 04:16:53 EDT
Koji scratchbuild for Rawhide:

* http://koji.fedoraproject.org/koji/taskinfo?taskID=4590277
Comment 4 Yuliya Algaer 2012-10-15 04:32:00 EDT
Ok, what are our next steps?
Comment 5 Peter Lemenkov 2012-10-15 04:33:43 EDT
(In reply to comment #4)
> Ok, what are our next steps?

Wait a moment - I'm manually inspecting ugene sources now.
Comment 6 Peter Lemenkov 2012-10-15 10:22:47 EDT
*** Bug 620005 has been marked as a duplicate of this bug. ***
Comment 7 Peter Lemenkov 2012-10-15 10:23:07 EDT
*** Bug 599952 has been marked as a duplicate of this bug. ***
Comment 8 Peter Lemenkov 2012-10-15 10:23:16 EDT
*** Bug 612333 has been marked as a duplicate of this bug. ***
Comment 9 Peter Lemenkov 2012-10-15 10:58:43 EDT
Sorry for the delay - I was busy with a random real-world proceedings.

For those who'd like to see a full context - this package was included in Fedora for some time but unfortunately was retired after Yegor Yefremov ceased his activity within our community (the pity of it!)

This package is (AFAIK) basically a continuation of Yegor's efforts. It is almost in the same shape that it was when Yegor left it. I see some somewhat relatively large issues which (if I were more partial towards the Fedora Guidelines) could be a stopper. I'd like to revive package first and address these issues (which slept through previous review) second.

So here is my official

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ rpmlint is NOT silent but almost all these messages are either a complaints about non-executable-script / wrong-script-interpreter (false positives triggered by UGENE file format), incorrect-fsf-address (triggered by the outdated licensing header in some files), spelling false positives, and messages triggered by a non-standard layout (usage of %{_libdir}/%{name}).

The latter I suppose should be improved/fixed upstream (I mean by you and your fellow colleagues) and I really won't insist on this now.

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license.

+/- The file, containing the text of the license(s) for the package, is included in %doc. Please also include LICENSE.3rd_party for now.

+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum ugene-1.11.2.tar.gz*
ba8c25a03eebd60730f5b9b5e08cd0e6d0cc1d5a9f965bc5870a5431c1c24296  ugene-1.11.2.tar.gz
ba8c25a03eebd60730f5b9b5e08cd0e6d0cc1d5a9f965bc5870a5431c1c24296  ugene-1.11.2.tar.gz.1
sulaco ~/rpmbuild/SOURCES: 

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji link above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files in some of the dynamic linker's default paths.

+/- The package shouldn't bundle copies of system libraries (CANNOT actually but in this very case I wouldn't insist assuming that you will fix/improve this). See my questions below.

0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No C/C++ header files.

+/- The package contains static libraries - they should be removed. If they are required for some operations (I really doubt that) they should be stored in a -static package. I personally advise you to remove them entirely - they do looks like a leftover.

0 No pkgconfig(.pc) files.

+/- The library files that end in .so (without suffix) should be stored in a -devel package. Well, I do believe that they should be simply removed (or even built w/o versioned so-names).

0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
+ The package includes a %{name}.desktop file, and this file is properly installed (and validated with desktop-file-validate) in the %install section.
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in rpm packages are valid UTF-8.


Almost finished. So far I've got the following questions/suggestions/requests:


* (Until we drop all bundled libs) please include LICENSE.3rd_party.
* What "UGENE_EXCLUDE_LIST_ENABLED" switch does? Are there any other compile-time switches?
* You may drop %defattr line from the %files section - it's no longer needed (even in EL5)
* Do you have plans to provide builds for EL5 or EL6? If no, then you may remove %clean section entirely, "rm -rf %{buildroot}" from %install and kill Buildroot directive as well.
* I really felt sad when I saw a bunch of bundled libraries residing within your tarball. You really should drop them and compile against system-wide ones. Is it possible? Did you patch them?
* The libraries - why not to install them into %{_libdir}? You will get rid of all these LD_LIBRARY_PATH shell magic? I understand - the only application which uses them is ugene itself, so someone might argue that it should go into named subdirectory rather than into system-wide directory.
* Remove (or explain why it's impossible) two static libs.
Comment 10 Peter Lemenkov 2012-10-15 13:38:58 EDT
I terribly, terribly sorry, but I accidentally mixed up Ivan Efremov, a previous ugene Fedora maintainer, with another one software developer with the identical surname. Please, s,Yegor,Ivan,g.
Comment 11 Peter Lemenkov 2012-10-15 13:41:57 EDT
Lifting FE-NEEDSPONSOR - I've just sponsored Yuliya Algaer.
Comment 12 Peter Lemenkov 2012-10-28 14:27:22 EDT
Yuliya, ping!
Comment 13 Yuliya Algaer 2012-11-02 04:06:00 EDT
Hello, Peter!

I'm so sorry for delay! We are prepared the new release and we are included your questions/suggestions/requests. 

Please, see new links:

Spec URL: http://ugene.unipro.ru/downloads/ugene.spec
SRPM URL: http://ugene.unipro.ru/downloads/ugene-1.11.3-1.fc17.src.rpm


I have one question about LD_LIBRARY_PATH:

You wrote:
>The libraries - why not to install them into %{_libdir}? You will get rid of all >these LD_LIBRARY_PATH shell magic? I understand - the only application which uses >them is ugene itself, so someone might argue that it should go into named >subdirectory rather than into system-wide directory.

My question is:

What would be better: add all libraries in the default folder or maybe there is another way to keep them in a separate folder without using LD_LIBRARY_PATH?
Comment 14 Yuliya Algaer 2012-11-07 02:15:15 EST
Peter, ping! What are our next steps?
Comment 15 Peter Lemenkov 2012-11-13 03:59:54 EST
(In reply to comment #14)
> Peter, ping! What are our next steps?

Ok, I've just rebuilt your latest package:

* http://koji.fedoraproject.org/koji/taskinfo?taskID=4682123

So here is the short summary of what was done in regards to my prevoius concerns:


===================================


* (Until we drop all bundled libs) please include LICENSE.3rd_party.


Done!


* What "UGENE_EXCLUDE_LIST_ENABLED" switch does? Are there any other compile-time switches?


See src/ugene_globals.pri. This just excludes these three modules - src/plugins/CoreTests, src/plugins/test_runner, src/plugins/perf_monitor, src/plugins/GUITestBase, and it looks pretty harmless to me.


* You may drop %defattr line from the %files section - it's no longer needed (even in EL5)


Done!


* Do you have plans to provide builds for EL5 or EL6? If no, then you may remove %clean section entirely, "rm -rf %{buildroot}" from %install and kill Buildroot directive as well.


Done as well.


* I really felt sad when I saw a bunch of bundled libraries residing within your tarball. You really should drop them and compile against system-wide ones. Is it possible? Did you patch them?


I'm mostly referring to src/libs_3rdparty/samtools, src/libs_3rdparty/sqlite3, and src/libs_3rdparty/zlib. The other two libraries - src/libs_3rdparty/gtest, which is used for testing, and src/libs_3rdparty/qscore, which isn't available in Fedora afaik, aren't concerning me too much.

While zlib is definitely not even touched during compilation, the first two are compiled into static libraries and the application is linked against them. As I said I won't insist on compiling against system-wide copies right now. But I really want you to do this in the future versions (as you already did with zlib, e.g. add a compile-time switch to allow building against system-wide copy). 


* The libraries - why not to install them into %{_libdir}? You will get rid of all these LD_LIBRARY_PATH shell magic? I understand - the only application which uses them is ugene itself, so someone might argue that it should go into named subdirectory rather than into system-wide directory.


Now you installed them into %{_libdir}/%{name} w/o soname suffixes. This looks much saner and I'll accept thet for now. However as a FutureFeature I still believe there is a room for improvement here.


* Remove (or explain why it's impossible) two static libs. 


Done. They were simply removed.


===================================


I don't see any other issues and assuming that this is a huge and complex application with some legacy which cannot be easily thrown away/fixed I declare this package as 


APPROVED.
Comment 16 Yuliya Algaer 2012-11-13 04:39:18 EST
Package Change Request
======================
Package Name: ugene 
New Branches: f17 f18 el6
Owners: yalgaer
InitialCC: iefremov 

Some time ago our package has been retired. Now package rereviewed by Peter Lemenkov. Please unretired our package.
Comment 17 Gwyn Ciesla 2012-11-13 08:21:28 EST
Git done (by process-git-requests).
Comment 18 Yuliya Algaer 2012-11-14 23:06:52 EST
Package Change Request
======================
Package Name: ugene 
New Branches: f19
Owners: yalgaer

I can't run building package with error: "package ugene is blocked for tag f19".
Comment 19 Gwyn Ciesla 2012-11-15 08:17:08 EST
devel == f19 and is automatic.
Comment 20 Yuliya Algaer 2012-11-19 05:41:43 EST
(In reply to comment #19)
> devel == f19 and is automatic.

ok, thank you!
Comment 21 Fedora Update System 2012-11-26 03:43:32 EST
ugene-1.11.3-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/ugene-1.11.3-1.fc18
Comment 22 Fedora Update System 2012-11-26 03:43:48 EST
ugene-1.11.3-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/ugene-1.11.3-1.fc17
Comment 23 Fedora Update System 2012-11-27 04:44:07 EST
ugene-1.11.3-1.fc18 has been pushed to the Fedora 18 testing repository.
Comment 24 Fedora Update System 2012-12-01 04:53:45 EST
ugene-1.11.3-1.fc18 has been pushed to the Fedora 18 stable repository.
Comment 25 Fedora Update System 2012-12-01 08:51:43 EST
ugene-1.11.3-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/ugene-1.11.3-2.fc17
Comment 26 Fedora Update System 2012-12-01 08:51:58 EST
ugene-1.11.3-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/ugene-1.11.3-2.fc18
Comment 27 Fedora Update System 2012-12-04 00:22:22 EST
ugene-1.11.3-2.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 28 Fedora Update System 2012-12-10 20:27:34 EST
ugene-1.11.3-2.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 29 Peter Lemenkov 2013-08-07 08:05:55 EDT
*** Bug 866321 has been marked as a duplicate of this bug. ***
Comment 30 Peter Lemenkov 2013-08-07 08:06:32 EDT
*** Bug 481717 has been marked as a duplicate of this bug. ***

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