This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 676941

Summary: Review Request: eiskaltdcpp-gtk - GTK+ frontend to DC++ client library eiskaltdcpp
Product: [Fedora] Fedora Reporter: Tirtha Chatterjee <tirtha.p.chatterjee>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED DEFERRED QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: bgeorges, fedora-package-review, martin.gieseking, msuchy, susi.lehtola
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard: NotReady
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-07-21 09:08:24 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449    

Description Tirtha Chatterjee 2011-02-11 18:02:07 EST
Spec URL: http://www.4shared.com/file/4Se1Dk4F/eiskaltdcpp-gtk.html
SRPM URL: http://www.4shared.com/file/sAFNlTul/eiskaltdcpp-gtk-220-1fc14src.html

Description: EiskaltDC++ is a cross-platform program that
uses the Direct Connect and ADC protocol.
It is compatible with other DC clients, such as the
original DC from Neomodus, DC++ and derivatives.
EiskaltDC++ also inter-operates with all common DC hub software. 

This package provides the GTK+ frontend to the EiskaltDC++ library.
Comment 1 Patryk Obara 2011-02-19 15:47:39 EST
(This is not normal review, because I am novice packager, but a bit of of
feedback):

1) host files somewhere other than 4shared.com - it's impossible to download them using wget :(
Public dropbox dir or hosting files on http://fedorapeople.org would work better :)

2) Source0 url: better use:
http://eiskaltdc.googlecode.com/files/eiskaltdcpp-%{version}.tar.bz2

3) unfortunately, %build step failed for me:

CMake Error at /usr/share/cmake/Modules/FindOpenSSL.cmake:93 (MESSAGE):
  Could NOT find OpenSSL
Call Stack (most recent call first):
  CMakeLists.txt:70 (find_package)

4) I don't think you need this:
%install
rm -rf %{buildroot}

buildroot is always cleaned before %install step

5) Why does %dir do? I can't find it in guidelines/documentation anywhere - I don't think it's necessary.

6) change %doc to
%doc COPYING LICENSE
You could also add ChangeLog files in doc

7) update date in %changelog, it says February 2010 ;)
Comment 2 Patryk Obara 2011-02-19 17:51:38 EST
Actually, I was wrong about point (4). By looking at different reviews I see that is required after all.
Comment 3 Susi Lehtola 2011-02-19 18:16:36 EST
(In reply to comment #2)
> Actually, I was wrong about point (4). By looking at different reviews I see
> that is required after all.

It is not required, the reviews you have looked at have been performed incorrectly, were done a long time ago or were targeting EPEL.

http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
http://fedoraproject.org/wiki/Packaging/Guidelines#.25clean
Comment 4 Patryk Obara 2011-02-19 18:30:32 EST
(In reply to comment #3)
> (In reply to comment #2)
> > Actually, I was wrong about point (4). By looking at different reviews I see
> > that is required after all.
> 
> It is not required, the reviews you have looked at have been performed
> incorrectly, were done a long time ago or were targeting EPEL.

I was misguided by last comment in this, quite recent, review: https://bugzilla.redhat.com/show_bug.cgi?id=673919
(there is no mention in there, that it targets EPEL)
> At the beginning of %install, each package MUST run rm -rf %{buildroot} - OK

Thanks for clarification, Jussi :)
Comment 5 Michael Schwendt 2011-04-01 12:57:37 EDT
Some more items:


> Requires:       gtk2 >= 2.10
> Requires:       glib2 >= 2.10
> Requires:       libglade2 >= 2.4
> Requires:       gettext
> Requires:       libeiskaltdcpp = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

For a Fedora Packager it is very important to understand rpmbuild's automatic dependencies on library SONAMEs.


> 5) What does %dir do?

It includes a directory (and just the dir, not any files within the dir) in the RPM package with proper ownership and permissions. 
https://fedoraproject.org/wiki/Packaging:UnownedDirectories
Under certain circumstances, %dir and recursive inclusion of a directory compete with eachother with regard to adding an entry for the directory to the package.


> %files -f %{name}.lang
> %{_datadir}/locale/*/LC_MESSAGES/eiskaltdcpp-gtk.mo

https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files


> %{_mandir}/man1/eiskaltdcpp-gtk.1.gz

Acceptable, but please notice that the gzip compression is applied by rpmbuild and may change. So, %{_mandir}/man1/eiskaltdcpp-gtk.1* would be safer.


> make install/strip DESTDIR=%{buildroot}

Please investigate what this "/strip" here does.


> rm -f %{buildroot}/%{_libdir}/libeiskaltdcpp.so.2.2
> rm -f %{buildroot}/%{_datadir}/locale/*/LC_MESSAGES/libeiskaltdcpp.mo

Two commands that ask for an explanation in a comment.


> %{_datadir}/applications/eiskaltdcpp-gtk.desktop

https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files

[...]

https://fedoraproject.org/wiki/Packaging:Guidelines
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
https://fedoraproject.org/wiki/PackageMaintainers
Comment 6 Jason Tibbitts 2012-05-09 18:18:58 EDT
No response from the submitter, and in addition I cannot download the spec or srpm at all.  Marking as notready; I'll close this if nothing reviewable appears soon.
Comment 7 Miroslav Suchý 2015-07-21 09:08:24 EDT
Closing per #6. Feel free to reopen if you want to continue.