Bug 1000683 - Review Request: gnome-maps - a simple map application for GNOME desktop
Review Request: gnome-maps - a simple map application for GNOME desktop
Status: CLOSED CURRENTRELEASE
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: 999153
Blocks: DebugInfo
  Show dependency treegraph
 
Reported: 2013-08-24 05:31 EDT by Elad Alfassa
Modified: 2013-11-21 13:43 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-11-21 13:43:27 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kalevlember: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Elad Alfassa 2013-08-24 05:31:50 EDT
Spec URL: http://elad.fedorapeople.org/reviews/gnome-maps/gnome-maps.spec
SRPM URL: http://elad.fedorapeople.org/reviews/gnome-maps/gnome-music-3.9.90-1.fc21.src.rpm
Description: a simple map application for GNOME desktop
Fedora Account System Username: elad

Note: has a runtime requirement on geoclue2 which is still waiting for review (bug #999153)
Comment 1 Christopher Meng 2013-08-24 05:35:19 EDT
1. Remove rm -rf $RPM_BUILD_ROOT.

2. Add icon cache refresh script for hicolor dir.
Comment 2 Elad Alfassa 2013-08-24 05:43:16 EDT
oops. Also forgot the glib schema compile scripts...


Fixed.
SRPM URL: http://elad.fedorapeople.org/reviews/gnome-maps/gnome-maps-3.9.90.1-2.fc20.src.rpm
Comment 3 Elad Alfassa 2013-08-24 12:23:37 EDT
Updated to latest upstream, added BuildArch: noarch

SRPM URL: http://elad.fedoraproject.org/reviews/gnome-maps/gnome-maps-3.9.90.2-1.fc20.src.rpm
Comment 4 Zeeshan Ali 2013-08-24 15:58:17 EDT
Could you please change the Summary to 'A map application for GNOME 3' or simply 'A map application for GNOME'?
Comment 5 Michael Schwendt 2013-08-24 16:38:11 EDT
It's widely accepted practise to omit leading articles, such as "An" and "A" to
shorten summaries even further:

  Summary: Map application for GNOME

That also looks better when displayed by package installer tools. https://fedoraproject.org/wiki/Examples_of_good_package_summaries

[...]

gnome-maps.src: W: summary-not-capitalized C simple map application for GNOME desktop

[...]

SRPM URL is invalid. s/fedoraproject/fedorapeople/ -> 
http://elad.fedorapeople.org/reviews/gnome-maps/gnome-maps-3.9.90.2-1.fc20.src.rpm

[...]

Run rpmlint (or rpmlint -i for more helpful output) on the src.rpm and all
built rpms. Feel free to ignore obvious false positives in the report, but fix
anything else. Preferably add a comment here about whether/when you think what
rpmlint reports is correct or incorrect.

[...]

> Requires:       geoclue2
> Requires:       geocode-glib

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
Comment 6 Elad Alfassa 2013-08-24 17:28:00 EDT
(In reply to Michael Schwendt from comment #5)
> [...]
> 
> gnome-maps.src: W: summary-not-capitalized C simple map application for
> GNOME desktop
Will fix soon
> 
> [...]
> 
> SRPM URL is invalid. s/fedoraproject/fedorapeople/ -> 
> http://elad.fedorapeople.org/reviews/gnome-maps/gnome-maps-3.9.90.2-1.fc20.
> src.rpm
> 
Oops, yeah, I should read what I write before I click save
> [...]
> 
> Run rpmlint (or rpmlint -i for more helpful output) on the src.rpm and all
> built rpms. Feel free to ignore obvious false positives in the report, but
> fix
> anything else. Preferably add a comment here about whether/when you think
> what
> rpmlint reports is correct or incorrect.
> 
> [...]
> 
> > Requires:       geoclue2
> > Requires:       geocode-glib
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
Since both geoclue2 and geocode-glib are new packages in Fedora, I feel like adding a version makes little to no sense - nobody will have an older version of these packages simply because older versions never existed within Fedora.
Comment 7 Michael Schwendt 2013-08-24 18:11:05 EDT
The important detail of that guideline is to add a comment that explains why the explicit Requires is needed.  For example, and since it's package names which are required, sum up what inside these packages is needed.
Comment 8 Elad Alfassa 2013-08-24 18:23:59 EDT
the explicit requires are needed because rpm doesn't have a mechanism to find requirements of JS applications automatically, this would be the same for Python apps, and if you look on other python or js packages in fedora you will find explicit requires without explanatory comments.

also, of course it's the package names that are listed, file-based requires are discoureged. 

I'm not going to add useless comments to the spec.
Comment 9 Michael Schwendt 2013-08-24 19:02:39 EDT
Of course they aren't added automatically, or else you would not add them explicitly. ;-)

However, Python packages in Fedora have "python" in their name (or "py"), so a dependency of one Python based package on another Python based package is self-explaining (a comment "we need this" would be superfluous indeed).

On the contrary, these two package names don't give a hint about what they may contain and whether they include JavaScript files or a DBus service that is needed. A simple comment would improve the packaging quality.
Comment 10 Elad Alfassa 2013-08-25 06:45:13 EDT
No. JS apps use normal shared libraries just like complied applications do, but they do it via GObject Introspection. What does it matter to other packagers who will read the spec if I need a shared library or a dbus service? (hint: it doesn't).
Comment 11 Michael Schwendt 2013-08-25 07:43:28 EDT
As pointed out before: package contents may change, files may move to other packages or subpackages, files may get renamed or deleted. That makes a dependency on a package name a weak dependency. A comment in the spec file can be helpful for other packagers, who examine inter-package dependencies with e.g. repoquery.

I've opened a thread on packaging list to raise some discussion about comments on explicit Requires and suggesting that the guidelines be enhanced.
Comment 12 Kalev Lember 2013-08-26 09:54:07 EDT
Taking for review.
Comment 13 Zeeshan Ali 2013-08-26 09:54:51 EDT
If package A requires package B, thats all the info you need. Comments are needed as much as it would be if its a C app/library. I think we are just bike-shedding a non-issue now.
Comment 14 Kalev Lember 2013-08-26 10:46:24 EDT
$ rpmlint gnome-maps-3.9.90.2-1.fc20.src.rpm \
          gnome-maps-3.9.90.2-1.fc20.noarch.rpm
gnome-maps.src: W: summary-not-capitalized C simple map application for GNOME desktop
gnome-maps.noarch: E: explicit-lib-dependency geocode-glib
gnome-maps.noarch: W: summary-not-capitalized C simple map application for GNOME desktop
gnome-maps.noarch: W: no-manual-page-for-binary gnome-maps
2 packages and 0 specfiles checked; 1 errors, 3 warnings.


Can you fix the summary as per Zeeshan's and Michael's suggestions and make rpmlint happy at the same time? "Map application for GNOME" sounds good to me.

The rest of the rpmlint warnings are harmless and can be ignored.


> +Source0: http://ftp.acc.umu.se/pub/GNOME/sources/gnome-maps/3.9/%{name}-%{version}.tar.xz

Would be better to use http://download.gnome.org/sources/gnome-maps/3.9/gnome-maps-%{version}.tar.xz for the URL instead of the ftp.acc.umu.se mirror.


> BuildArch: noarch

It can't be noarch because /usr/bin/gnome-maps includes full paths to the GI typelibs under $libdir and these paths would differ between multilib arches.


> Requires:       geoclue2
> Requires:       geocode-glib

It would also have to depend on 'gjs' + all the packages that ship the following typelibs:

$ rpm -ql gnome-maps | grep '\.js$' | xargs grep -h 'imports\.gi\.' | sort | uniq 
const Champlain = imports.gi.Champlain;
const Clutter = imports.gi.Clutter;
const Cogl = imports.gi.Cogl;
const GLib = imports.gi.GLib;
const GObject = imports.gi.GObject;
const Gdk = imports.gi.Gdk;
const GdkPixbuf = imports.gi.GdkPixbuf;
const Geocode = imports.gi.GeocodeGlib;
const Gio = imports.gi.Gio;
const Gtk = imports.gi.Gtk;
const GtkChamplain = imports.gi.GtkChamplain;
const GtkClutter = imports.gi.GtkClutter;


> %description
> GNOME Maps is a simple map application for the GNOME desktop

Sorry for nitpicking, but can you add a full stop to the end of the sentence please?


> gtk-update-icon-cache %{_datadir}/icons/hicolor >&/dev/null || :
> /usr/bin/glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || :

Perhaps remove the path from /usr/bin/glib-compile-schemas as well so that they'd look consistent.


Also, can you make it obsolete emerillon, please? It's getting retired from F20+ and would be nice to have an upgrade path.

Something like this:

%define emerillon_version 0.1.90-15
Obsoletes: emerillon < %{emerillon_version}
Obsoletes: emerillon-devel < %{emerillon_version}
Obsoletes: emerillon-vala < %{emerillon_version}
Comment 15 Elad Alfassa 2013-08-27 10:29:09 EDT
Fixed all issues.

SRPM URL: http://elad.fedorapeople.org/reviews/gnome-maps/gnome-maps-3.9.90.2-2.fc20.src.rpm
Comment 16 Kalev Lember 2013-08-27 10:52:01 EDT
Fedora review gnome-maps-3.9.90.2-2.fc20.src.rpm 2013-08-27

$ rpmlint gnome-maps-3.9.90.2-2.fc20.src.rpm \
          gnome-maps-3.9.90.2-2.fc20.x86_64.rpm
gnome-maps.x86_64: E: explicit-lib-dependency geocode-glib
gnome-maps.x86_64: E: explicit-lib-dependency libchamplain
gnome-maps.x86_64: E: explicit-lib-dependency libchamplain-gtk
gnome-maps.x86_64: W: obsolete-not-provided emerillon
gnome-maps.x86_64: W: obsolete-not-provided emerillon-devel
gnome-maps.x86_64: W: obsolete-not-provided emerillon-vala
gnome-maps.x86_64: E: no-binary
gnome-maps.x86_64: W: no-manual-page-for-binary gnome-maps
2 packages and 0 specfiles checked; 4 errors, 4 warnings.


+ OK
! needs attention

+ rpmlint warnings / errors are harmless and can be ignored
+ The package is named according to Fedora packaging guidelines
+ The spec file name matches the base package name.
+ The package meets the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ 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:
  a183744d6c78086c29ddf57816967ad2  gnome-maps-3.9.90.2.tar.xz
  a183744d6c78086c29ddf57816967ad2  Download/gnome-maps-3.9.90.2.tar.xz
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a The spec file handles locales properly
n/a ldconfig in %post and %postun
+ Package does not bundle copies of system libraries
n/a Package isn't relocatable
+ Package owns all the directories it creates
+ No duplicate files in %files
+ Permissions are properly set
+ Consistent use of macros
+ The 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 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
+ Doesn't own files or directories already owned by other packages
+ Filenames are valid UTF-8

Looks good to me, just two small things with the runtime requires:

 1) Missing gjs dep, can you add either 'Requires: gjs' or 'Requires: /usr/bin/gjs-console' before importing?

 2) It would be safer to tighten the library requires with %{?_isa} so that it doesn't accidentally pull in the wrong multilib arch. Yum's depsolver gets easily confused.

APPROVED
Comment 17 Elad Alfassa 2013-08-27 12:11:29 EDT
New Package SCM Request
=======================
Package Name: gnome-maps
Short Description: Map application for GNOME
Owners: kalev zeenix elad
Branches: f20
InitialCC:
Comment 18 Jon Ciesla 2013-08-27 12:26:00 EDT
Git done (by process-git-requests).
Comment 19 Ville Skyttä 2013-08-31 15:52:54 EDT
-debuginfo package is empty, needs to be explicitly disabled if there's nothing to extract debuginfo from.
Comment 20 Elad Alfassa 2013-08-31 18:08:51 EDT
(In reply to Ville Skyttä from comment #19)
> -debuginfo package is empty, needs to be explicitly disabled if there's
> nothing to extract debuginfo from.
How do I do that?
Comment 22 Ville Skyttä 2013-09-08 03:29:42 EDT
Elad: ping?
Comment 23 Elad Alfassa 2013-09-08 07:46:18 EDT
Sorry for taking this long. Building a fixed package in rawhide now, will build for f20 too asap.
Comment 24 Elad Alfassa 2013-11-21 13:43:27 EST
Oh, wow, this one is still open? I fixed it long ago. Closing.

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