Bug 754023 - Review Request: mumpot - GTK mapping application
Summary: Review Request: mumpot - GTK mapping application
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jerry James
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-15 07:23 UTC by Volker Fröhlich
Modified: 2013-01-18 18:47 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-01-18 18:47:58 UTC
Type: ---
Embargoed:
loganjerry: fedora-review?


Attachments (Terms of Use)

Description Volker Fröhlich 2011-11-15 07:23:03 UTC
Spec URL: http://www.geofrogger.net/review/mumpot.spec
SRPM URL: http://www.geofrogger.net/review/mumpot-0.6-1.fc16.src.rpm

Description:

Mumpot is a GTK mapping application with simple editing features
(for editing OpenStreetMap data) mainly (but not only) for use 
with mobile Linux equipped devices, tested on a GPE installation
on an iPAQ2200 and on the Openmoko platform.

Mumpot can connect to a GPS, which provides NMEA-Data. It can down-
and upload OSM data and also search for shortest paths.

Comment 1 Michael Schwendt 2011-11-19 19:28:12 UTC
* https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage


> %patch0 -p1 -b .desktop

Caution! The '-b .desktop' option creates a copy of the unpatched file with '.desktop' at its end of the file name. Therefore it bears the risk of creating confusion for anything that would work on a '*.desktop' wildcard.
Safer would be something less ambiguous, such as:

  %patch0 -p1 -b .desktop-file


> %{_datadir}/%{name}/tagpresets

Just by looking at the spec file %files section, this entry is evidence that the %_datadir/%name directory is not included:
https://fedoraproject.org/wiki/Packaging:UnownedDirectories


> %{_mandir}/man1/%{name}-*.1.*
> %{_mandir}/man1/download_osm.1.*
> %{_mandir}/man1/osm2places.1.*
> %{_mandir}/man1/%{name}.1.*
> %{_mandir}/man1/get_maprect.1.*
> %{_mandir}/man1/simplify-gps.1.*

Looks funny, but no blocker here. Only comments:

 - Manual pages are compressed by rpmbuild, so you could append '1*' instead of '1.*' to also handle the case of packaging uncompressed man pages.

 - Listing individual file names with only limited use of macros and '*' wildcards can be beneficial in some cases (and elsewhere in the %files section, e.g. as a guard of the type "I want this particular file to exist"), but is it important here to enter specific file names? This wouldn't be convenient with packages that include many more man pages. Also, your usage of '%{name}-*' in the %files section implies that any files matching that pattern can go missing or return in arbitrary updates of the package without terminating the build. Are the other manuals much more important as not to terminate the build? Would

  %{_mandir}/man1/*.1*

not be good enough?


* Format string warnings in the build log on x86_64!

Comment 2 Volker Fröhlich 2012-01-08 04:47:50 UTC
Spec URL: http://www.geofrogger.net/review/mumpot.spec
SRPM URL: http://www.geofrogger.net/review/mumpot-0.6-2.fc15.src.rpm

I hope, I have addressed all issues.

* Sun Nov 20 2011 Volker Fröhlich <volker27> - 0.6-2
- Rename patch1 backup file
- BR desktop-file-utils and validate desktop file
- Generalize files section
- Solve unsigned int to digit formatting problem (Patch #3)

Comment 3 Jerry James 2012-02-15 22:54:30 UTC
Taking for review.

Comment 4 Jerry James 2012-02-15 23:38:54 UTC
This package fails to build in Rawhide, probably due to the recent libpng update.  The failing part of the build is this:

gcc -DLOCALEDIR=\"/usr/share/locale\" -DMUMPOT_DATADIR=\"/usr/share/mumpot\" -DHAVE_CONFIG_H -I. -I.. -pthread -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng15     -I/usr/include/libxml2   -I../pixmaps -I../intl    -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -c mapconfig.c
png_io.c: In function 'save_pinfo':
png_io.c:79:23: error: dereferencing pointer to incomplete type
png_io.c: In function 'load_gfxfile':
png_io.c:171:23: error: dereferencing pointer to incomplete type
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 *'
png_io.c:149:8: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
make[3]: *** [png_io.o] Error 1

Comment 5 Volker Fröhlich 2012-02-17 13:00:11 UTC
Spec URL: http://www.geofrogger.net/review/mumpot.spec
SRPM URL: http://www.geofrogger.net/review/mumpot-0.6-3.fc16.src.rpm

- Add patch to use up-to-date libpng API

Comment 6 Jerry James 2012-02-17 16:04:16 UTC
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

Comment 7 Volker Fröhlich 2012-02-23 21:10:52 UTC
I've been working on it, but solving this raises other warnings. It will take me some time until I can finish this.

Comment 8 Jerry James 2012-02-24 20:11:30 UTC
No problem.  I'm patient. :-)

Comment 9 Jerry James 2012-08-03 16:11:15 UTC
Hi Volker, any progress?

Comment 10 Volker Fröhlich 2012-08-15 14:37:48 UTC
I haven't looked into it since. We can close the ticket for now, if you like. Sorry for the long delay.

Comment 11 Jerry James 2012-08-15 14:41:15 UTC
If you think you'll have a candidate package by, say, the end of the year, then I'm willing to keep waiting.  If you think it will be 2013 or beyond, though, we should probably close it for now.

Comment 12 Jerry James 2013-01-18 18:41:59 UTC
Hi Volker.  Any updates on this package?  If no progress is being made, we should probably close this bug.

Comment 13 Volker Fröhlich 2013-01-18 18:47:58 UTC
Yes, we'll close it.

Thank you for the reminder and your patience!


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