Bug 866325

I'll review it (and I will sponsor you).

I requested for sponsorship, FAS name: yalgaer. 

Please, approve me as a member of the packager and provenpackager groups.

Koji scratchbuild for Rawhide:

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

Ok, what are our next steps?

(In reply to comment #4)
> Ok, what are our next steps?

Wait a moment - I'm manually inspecting ugene sources now.

*** Bug 620005 has been marked as a duplicate of this bug. ***

*** Bug 599952 has been marked as a duplicate of this bug. ***

*** Bug 612333 has been marked as a duplicate of this bug. ***

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.

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.

Lifting FE-NEEDSPONSOR - I've just sponsored Yuliya Algaer.

Yuliya, ping!

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?

Peter, ping! What are our next steps?

(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.

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.

Git done (by process-git-requests).

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".

devel == f19 and is automatic.

(In reply to comment #19)
> devel == f19 and is automatic.

ok, thank you!

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

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

ugene-1.11.3-1.fc18 has been pushed to the Fedora 18 testing repository.

ugene-1.11.3-1.fc18 has been pushed to the Fedora 18 stable repository.

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

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

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.

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.

*** Bug 866321 has been marked as a duplicate of this bug. ***

*** Bug 481717 has been marked as a duplicate of this bug. ***