Bug 525927

Taking for review.

I have never dealt with orphaned packages before, but it appears that you have already uploaded a newer version into CVS. I'll base my review on the following files instead:

Spec URL: http://cvs.fedoraproject.org/viewvc/rpms/incollector/devel/incollector.spec?view=markup&pathrev=incollector-1_2-1_fc13
SRPM URL: http://kojipkgs.fedoraproject.org/packages/incollector/1.2/1.fc13/src/incollector-1.2-1.fc13.src.rpm

Yes, the old one was really old. And there was some build problem with devel (mass rebuild IIRC). Sorry, i forgot to update the spec and srpm here. Will do that today.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Thomas, I already got the spec and srpm from pkgdb / koji, no need to duplicate those here.

Fedora review incollector-1.2-1.fc13.src.rpm 2009-12-01

+ OK
! needs attention

rpmlint says:
incollector.i686: E: no-binary
incollector.i686: W: only-non-binary-in-usr-lib
2 packages and 1 specfiles checked; 1 errors, 1 warnings.

All these rpmlint warnings/errors are expected for mono packages and can be ignored.

+ rpmlint output
+ package is named according to the Package Naming Guidelines
+ specfile name matches base package name
+ package meets Packaging Guidelines
+ The stated license (GPLv2+) is a Fedora approved license
! The license doesn't match actual package license

Every source file contains the following lines:
 *  Copyright (C) 2006-2007 Marcin Krystian Krzywonos
 *  License: GNU/GPL version 2

I think this means that the license tag should read 'GPLv2'

+ The package contains the license file (COPYING)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
6701ac13da29119cd6719be3edcf30aa  incollector-1.2.tar.gz
6701ac13da29119cd6719be3edcf30aa  Download/incollector-1.2.tar.gz
+ Package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
+ Spec file handles locales properly
n/a binary RPM with shared library files must call ldconfig in %post and
    %postun
+ Package doesn't bundle copies of system libraries
+ Does not use Prefix: /usr
+ Package owns all directories it creates
+ No duplicate files in %files
+ Proper permissions and %files has %defattr
+ %clean contains rm -rf %{buildroot}
+ Consistent use of macros
+ Package must contain code or permissible content
+ Package must contain code or permissible content
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc should not affect package
n/a Header files should be in -devel
n/a Static libraries should be in -static
n/a Packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
n/a Library files that end in .so must go in a -devel package
n/a -devel must require the fully versioned base
+ Packages should not contain libtool .la files
+ Proper .desktop file handling
+ Package doesn't own files or directories already owned by other packages
+ %install begins with rm -rf %{buildroot}
+ all filename are valid UTF-8


! The following pushd/popd commands in the spec file without anything in between seem useless and should be removed:
  pushd po
  popd

! You have the following sed command to fix lib64 dir:
  sed -i 's|/usr/lib|%{_libdir}|' %{name}

I think a proper way to do that is fix script.in instead (that's something you could also send upstream):
-exec @MONO@ @prefix@/lib/incollector/incollector.exe $MONO_EXTRA_ARGS "$@"
+exec @MONO@ @pkglibdir@/incollector.exe $MONO_EXTRA_ARGS "$@"

Attaching the patch to the bug report in a separate file too.

! Consider removing %{_datadir}/pixmaps/%{name}.ico at the end of %install. I very much doubt anything uses .ico files in Fedora when there's a matching .png file in %{_datadir}/pixmaps/.

Created attachment 375079 [details]
Patch to fix hardcoded @prefix@/lib/ dir in wrapper script

(In reply to comment #3)
Thanks for the review Kalev.

> + The stated license (GPLv2+) is a Fedora approved license
> ! The license doesn't match actual package license
> 
> Every source file contains the following lines:
>  *  Copyright (C) 2006-2007 Marcin Krystian Krzywonos
>  *  License: GNU/GPL version 2
> 
> I think this means that the license tag should read 'GPLv2'

Some of the source, like the file 'missing' and 'config.guess' are GPLv2+.

> ! The following pushd/popd commands in the spec file without anything in
> between seem useless and should be removed:
>   pushd po
>   popd

Removed.
 
> ! You have the following sed command to fix lib64 dir:
>   sed -i 's|/usr/lib|%{_libdir}|' %{name}
> 
> I think a proper way to do that is fix script.in instead (that's something you
> could also send upstream):
> -exec @MONO@ @prefix@/lib/incollector/incollector.exe $MONO_EXTRA_ARGS "$@"
> +exec @MONO@ @pkglibdir@/incollector.exe $MONO_EXTRA_ARGS "$@"
> 
> Attaching the patch to the bug report in a separate file too.

Thanks for the patch. It's not a blocker to use sed, though i changed it to use patch since there was already a patch, for consistency.
 
> ! Consider removing %{_datadir}/pixmaps/%{name}.ico at the end of %install. I
> very much doubt anything uses .ico files in Fedora when there's a matching .png
> file in %{_datadir}/pixmaps/.  

The ico is used inside the app it seems. Removing it breaks the compiling. I dont want to patch the software to make it use a png.

Spec URL: http://thomasj.fedorapeople.org/reviews/incollector.spec
SRPM URL: http://thomasj.fedorapeople.org/reviews/incollector-1.2-2.fc11.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=1845565

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Changed the license to read GPLv2

Spec URL: http://thomasj.fedorapeople.org/reviews/incollector.spec
SRPM URL:http://thomasj.fedorapeople.org/reviews/incollector-1.2-2.fc11.src.rpm


-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Man.. Coffee is missing:

SRPM URL:http://thomasj.fedorapeople.org/reviews/incollector-1.2-3.fc11.src.rpm


-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

(In reply to comment #5)
> Thanks for the patch. It's not a blocker to use sed, though i changed it to use
> patch since there was already a patch, for consistency.

Of course, using sed is no problem. However, the sed command was just a workaround. The reason why I came up with the patch is that it fixes the root cause, and I am sure upstream is glad to apply it. For you personally using sed is probably easier, but if you submit the patch to upstream, you might be able to eventually remove the workaround in a future release.


> > ! Consider removing %{_datadir}/pixmaps/%{name}.ico at the end of %install. I
> > very much doubt anything uses .ico files in Fedora when there's a matching .png
> > file in %{_datadir}/pixmaps/.  
> 
> The ico is used inside the app it seems. Removing it breaks the compiling. I
> dont want to patch the software to make it use a png.

Yes, the .ico file gets embedded in the .exe file during %build section. However, what I was saying is that you might want to remove it at the end of %install from %{buildroot} (compiling is all done by that time). As I understand it, the .ico file gets embedded in the .exe during build, and nothing else uses it afterwards in %{_datadir}/pixmaps/. The .png is used for desktop integration, but since .ico files are mostly Windows-specific, I very much doubt Gnome or KDE would load them, especially if the .png file is present. There's a small chance that incollector itself might load the .ico at runtime, but grepping through the source gave me the impression that this is not the case.

But this is nothing serious, and I am not sure if it's even worth removing it. Just pointed it out for you.


APPROVED