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 Review | Assignee: | 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: | rawhide | CC: | 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 13:08:24 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 201449 |
Description
Tirtha Chatterjee
2011-02-11 23:02:07 UTC
(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 ;) Actually, I was wrong about point (4). By looking at different reviews I see that is required after all. (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 (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 :) 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 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. Closing per #6. Feel free to reopen if you want to continue. |