This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 525927 - Review Request: incollector - Information collector for various kinds of information
Review Request: incollector - Information collector for various kinds of inf...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kalev Lember
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-27 06:17 EDT by Thomas Janssen
Modified: 2009-12-04 10:38 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-04 10:38:59 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kalevlember: fedora‑review+


Attachments (Terms of Use)
Patch to fix hardcoded @prefix@/lib/ dir in wrapper script (369 bytes, patch)
2009-12-01 10:02 EST, Kalev Lember
no flags Details | Diff

  None (edit)
Description Thomas Janssen 2009-09-27 06:17:15 EDT
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.
Comment 1 Kalev Lember 2009-12-01 08:35:10 EST
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
Comment 2 Thomas Janssen 2009-12-01 09:28:12 EST
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
Comment 3 Kalev Lember 2009-12-01 09:58:15 EST
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/.
Comment 4 Kalev Lember 2009-12-01 10:02:39 EST
Created attachment 375079 [details]
Patch to fix hardcoded @prefix@/lib/ dir in wrapper script
Comment 5 Thomas Janssen 2009-12-03 04:53:16 EST
(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
Comment 6 Thomas Janssen 2009-12-03 05:03:37 EST
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
Comment 7 Thomas Janssen 2009-12-03 05:04:17 EST
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
Comment 8 Kalev Lember 2009-12-03 05:37:08 EST
(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

Note You need to log in before you can comment on or make changes to this bug.