[Bug 754023] Review Request: Mumpot - GTK mapping application

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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=754023

--- Comment #6 from Jerry James <loganjerry@xxxxxxxxx> 2012-02-17 11:04:16 EST ---
I think that mumpot-0.6-fprint.patch is not quite right.  I see these warnings
on an i386 build:

osm_parse.c: In function 'mem_stats':
osm_parse.c:1063:69: warning: format '%lu' expects argument of type 'long
unsigned int', but argument 3 has type 'unsigned int' [-Wformat]
osm_parse.c:1079:68: warning: format '%lu' expects argument of type 'long
unsigned int', but argument 3 has type 'unsigned int' [-Wformat]

Other compiler warnings that should be looked at (I have not checked whether
they represent actual problems or not):

png_io.c: In function 'load_gfxfile':
png_io.c:190:16: warning: passing argument 3 of 'png_get_IHDR' from
incompatible pointer type [enabled by default]
In file included from png_io.c:17:0:
/usr/include/libpng15/png.h:2153:22: note: expected 'png_uint_32 *' but
argument is of type 'long unsigned int *'
png_io.c:190:16: warning: passing argument 4 of 'png_get_IHDR' from
incompatible pointer type [enabled by default]
In file included from png_io.c:17:0:
/usr/include/libpng15/png.h:2153:22: note: expected 'png_uint_32 *' but
argument is of type 'long unsigned int *'

get_maprect.c: In function 'main':
get_maprect.c:139:10: warning: 'height' may be used uninitialized in this
function [-Wmaybe-uninitialized]
get_maprect.c:139:10: warning: 'width' may be used uninitialized in this
function [-Wmaybe-uninitialized]
get_maprect.c:132:26: warning: 'yoffset' may be used uninitialized in this
function [-Wmaybe-uninitialized]
get_maprect.c:139:28: warning: 'xoffset' may be used uninitialized in this
function [-Wmaybe-uninitialized]

The SVG icon is stored in the wrong place.  There is no such thing as
%{_datadir}/application/pixmaps in Fedora (check with repoquery).  There is
%{_datadir}/pixmaps, but %{_datadir}/icons/hicolor/scalable/apps is preferred. 
If you store the icon in the latter, also Require hicolor-icon-theme and see
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache.  See the
thread starting here:
http://lists.fedoraproject.org/pipermail/devel/2012-January/161076.html.

Also, you may need to change "Mumpot" to "mumpot" in the name of this bug
before an SCM request will be acted on.  I seem to remember differing
capitalization causing a problem with a previous review I did.

+: OK
-: must be fixed
=: should be fixed (at your discretion)
N: not applicable

MUST:
[+] rpmlint output:
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
[+] follows package naming guidelines
[+] spec file base name matches package name
[=] package meets the packaging guidelines

You SHOULD include more information about the upstream status of your patches:
https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Consider preserving the timestamp on the manpage whose encoding is changed
(https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps).  For example:

iconv -f iso8859-1 -t utf-8 doc/get_maprect.1 > doc/get_maprect.1.conv
touch -r doc/get_maprect.1 doc/get_maprect.1.conv
mv doc/get_maprect.1.conv doc/get_maprect.1

[+] package uses a Fedora approved license
[+] license field matches the actual license
[+] license file is included in %doc
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sum is 9a0409c39e49c45cea3160c7ec7fe976 for both
[+] package builds on at least one primary arch (tried x86_64)
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires:
[+] spec file handles locales properly
[N] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[+] no relocatable packages
[-] package owns all directories that it creates:
/usr/share/application/pixmaps is created but not owned; see the icon
discussion above
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[+] no runtime dependencies in %doc
[N] header files in -devel
[N] static libraries in -static
[N] .so in -devel
[N] -devel requires main package
[+] package contains no libtool archives
[+] package contains a desktop file, uses desktop-file-install/-validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[N] query upstream for license text
[N] description and summary contain available translations
[+] package builds in mock: tried fedora-rawhide-i386
[+] package builds on all supported arches: tried i386 and x86_64
[+] package functions as described: light testing only
[+] sane scriptlets
[N] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[+] package contains man pages for binaries/scripts

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review



[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]