[Bug 525927] New: Review Request: incollector - Information collector for various kinds of information
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: incollector - Information collector for various kinds of information
https://bugzilla.redhat.com/show_bug.cgi?id=525927
Summary: Review Request: incollector - Information collector for various kinds of information Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: thomasj@fedoraproject.org QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
Spec URL: http://thomasj.fedorapeople.org/reviews/incollector.spec SRPM URL: http://thomasj.fedorapeople.org/reviews/incollector-1.0-7.fc10.src.rpm Description: Incollector is an application to collect various kinds of information (like notes, conversation logs, quotes, serial numbers, source code, web addresses, words). All the entries can be tagged, so you can find them very easily. There are also search folders which allows you to search for entries by specified criteria. You can also export (and import, of course) entries to an external file.
This is a review for a orphaned package last updated 5 month's ago. https://fedoraproject.org/wiki/Orphaned_package_that_need_new_maintainers I'm the new maintainer.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
Kalev Lember kalev@smartlink.ee changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kalev@smartlink.ee AssignedTo|nobody@fedoraproject.org |kalev@smartlink.ee Flag| |fedora-review?
--- Comment #1 from Kalev Lember kalev@smartlink.ee 2009-12-01 08:35:10 EDT --- 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?... SRPM URL: http://kojipkgs.fedoraproject.org/packages/incollector/1.2/1.fc13/src/incoll...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
--- Comment #2 from Thomas Janssen thomasj@fedoraproject.org 2009-12-01 09:28:12 EDT --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
--- Comment #3 from Kalev Lember kalev@smartlink.ee 2009-12-01 09:58:15 EDT --- 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/.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
--- Comment #4 from Kalev Lember kalev@smartlink.ee 2009-12-01 10:02:39 EDT --- Created an attachment (id=375079) --> (https://bugzilla.redhat.com/attachment.cgi?id=375079) Patch to fix hardcoded @prefix@/lib/ dir in wrapper script
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
--- Comment #5 from Thomas Janssen thomasj@fedoraproject.org 2009-12-03 04:53:16 EDT --- (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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
--- Comment #6 from Thomas Janssen thomasj@fedoraproject.org 2009-12-03 05:03:37 EDT --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
--- Comment #7 from Thomas Janssen thomasj@fedoraproject.org 2009-12-03 05:04:17 EDT --- Man.. Coffee is missing:
SRPM URL:http://thomasj.fedorapeople.org/reviews/incollector-1.2-3.fc11.src.rpm
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
Kalev Lember kalev@smartlink.ee changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #8 from Kalev Lember kalev@smartlink.ee 2009-12-03 05:37:08 EDT --- (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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=525927
Thomas Janssen thomasj@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution| |NEXTRELEASE
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla