Bug 999153 - Review Request: geoclue2 - Geolocation service
Review Request: geoclue2 - Geolocation service
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Haïkel Guémar
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 1000683
  Show dependency treegraph
 
Reported: 2013-08-20 16:08 EDT by Kalev Lember
Modified: 2013-08-26 09:48 EDT (History)
6 users (show)

See Also:
Fixed In Version: geoclue2-1.99.2-3.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-26 09:48:24 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
karlthered: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Kalev Lember 2013-08-20 16:08:07 EDT
Spec URL: http://kalev.fedorapeople.org/geoclue2.spec
SRPM URL: http://kalev.fedorapeople.org/geoclue2-1.99.1-1.fc19.src.rpm
Description:
Geoclue is a D-Bus service that provides location information. The primary goal
of the Geoclue project is to make creating location-aware applications as
simple as possible, while the secondary goal is to ensure that no application
can access location information without explicit permission from user.

Fedora Account System Username: kalev
Comment 1 Kalev Lember 2013-08-21 10:10:00 EDT
* Wed Aug 21 2013 Kalev Lember <kalevlember@gmail.com> - 1.99.1-2
- Ship geoip-update in -server subpackage

Spec URL: http://kalev.fedorapeople.org/geoclue2.spec
SRPM URL: http://kalev.fedorapeople.org/geoclue2-1.99.1-2.fc20.src.rpm
Comment 2 Kalev Lember 2013-08-21 20:00:08 EDT
* Wed Aug 21 2013 Kalev Lember <kalevlember@gmail.com> - 1.99.1-3
- Include geoip-lookup in the -server subpackage as well

Spec URL: http://kalev.fedorapeople.org/geoclue2.spec
SRPM URL: http://kalev.fedorapeople.org/geoclue2-1.99.1-3.fc20.src.rpm
Comment 3 Baptiste Mille-Mathias 2013-08-22 16:35:02 EDT
Hi Kalev,

(I'm asked to make some review to be accepted as packager)

Could the description be updated to state the package geoclue2 is an incompatible evolution of geoclue. Does it make sense ?
The file demo/where-am-i.c has a licence  LGPL (v2.1 or later), so it has to be added to the Licence field, right ?

For now it all I have found to comment :)

Regards.
Comment 4 Haïkel Guémar 2013-08-22 18:29:28 EDT
As Baptiste sponsor, i'll take responsibility of validating this review as soon as he finish his informal review (which shouldn't take long from i've heard from him and my POV)

@kalev: if you're in a hurry, feel free to ping me about it.
Comment 5 Zeeshan Ali 2013-08-22 20:47:13 EDT
(In reply to Baptiste Mille-Mathias from comment #3)
> Hi Kalev,
> 
> (I'm asked to make some review to be accepted as packager)
> 
> Could the description be updated to state the package geoclue2 is an
> incompatible evolution of geoclue. Does it make sense ?
> The file demo/where-am-i.c has a licence  LGPL (v2.1 or later), so it has to
> be added to the Licence field, right ?

Ouch, that is an oversight. We recently relicensed from LGPL to GPL and I guess I forgot to update header on this file. I'll fix this now but please ignore this header for now.
Comment 6 Zeeshan Ali 2013-08-22 21:12:59 EDT
Kalev,

You might want to remove the -devel package. It seems we were not supposed to provide a library: https://bugs.freedesktop.org/show_bug.cgi?id=68439#c5
Comment 7 Kalev Lember 2013-08-23 05:59:59 EDT
(In reply to Baptiste Mille-Mathias from comment #3)
> Hi Kalev,
> 
> (I'm asked to make some review to be accepted as packager)

Hi Baptiste,

Welcome to packaging and feel free to post comments here and point out mistakes. Thanks!

> Could the description be updated to state the package geoclue2 is an
> incompatible evolution of geoclue. Does it make sense ?

Well, we're planning to kill off the original geoclue from Fedora soon, perhaps even in a few months (before the F20 final release), depending on how fast all the apps can get ported. So there won't be parallel installable geoclue/geoclue2 packages in long term, and I'm not sure it's worth mentioning the old geoclue package if it's no longer available in Fedora repositories.

All the apps and libraries that currently use geoclue won't get ported at once, though, so we need a period of time where both are parallel installable to allow porting apps one by one.


> The file demo/where-am-i.c has a licence  LGPL (v2.1 or later), so it has to
> be added to the Licence field, right ?

Zeeshan already commented on this, but I'll provide another perspective.

The License: tag in packages covers the license of the _binary_ rpms, as opposed to the _source_ package. As for this package, we aren't including demo/where-am-i.c in the binary rpms, so it doesn't have to be mentioned in the License: tag either. But if it was included in the package, you'd be absolutely correct.
Comment 8 Kalev Lember 2013-08-23 06:05:21 EDT
(In reply to Zeeshan Ali from comment #6)
> You might want to remove the -devel package. It seems we were not supposed
> to provide a library: https://bugs.freedesktop.org/show_bug.cgi?id=68439#c5

Sure, but let's do this upstream first and get a new release out that doesn't include the shared library.
Comment 9 Peter Robinson 2013-08-23 06:18:47 EDT
Wouldn't we be better off just updating the existing geoclue package as it's dead and basically not used by much at all anyway?
Comment 10 Kalev Lember 2013-08-23 06:35:10 EDT
Sure, but in that case the existing geoclue package should be first renamed to geoclue0 so that packages that link with it keep working (and all such packages would need updating to BR geoclue0-devel instead of geoclue).

I wouldn't mind doing it that way, but it's a bit of work for whoever wants to rename the old package out of the way. Adding new geoclue2 is less work and less disruption for existing stuff.

We need to have these parallel installable for a time to be able to move forward with gnome-maps without having to wait on other apps getting ported first. I'd like to get gnome-maps (and geoclue2 and geocode-glib) in by the time of the F20 Alpha Freeze, which is on 2013-09-03, so that we can ship it on the Desktop live media.
Comment 11 Peter Robinson 2013-08-23 06:51:56 EDT
(In reply to Kalev Lember from comment #10)
> Sure, but in that case the existing geoclue package should be first renamed
> to geoclue0 so that packages that link with it keep working (and all such
> packages would need updating to BR geoclue0-devel instead of geoclue).
>
> I wouldn't mind doing it that way, but it's a bit of work for whoever wants
> to rename the old package out of the way. Adding new geoclue2 is less work
> and less disruption for existing stuff.

repoquery --whatrequires libgeoclue.so.0
geoclue-devel-0:0.12.99-4.fc19.i686
libwebkit2gtk-0:2.0.4-1.fc19.i686
webkitgtk-0:2.0.4-1.fc19.i686
webkitgtk3-0:2.0.4-1.fc19.i686
[root@neo ~]# repoquery --whatrequires geoclue
emerillon-0:0.1.90-12.fc19.x86_64
empathy-0:3.8.3-2.fc19.x86_64
libwebkit2gtk-0:2.0.4-1.fc19.x86_64
python-geoclue-0:0.1.0-6.fc19.noarch
redshift-0:1.7-5.fc19.x86_64
vfrnav-0:20130801-1.fc19.x86_64
webkitgtk-0:2.0.2-2.fc19.i686
webkitgtk3-0:2.0.4-1.fc19.x86_64

Of the above the presumably webkit/empathy will be ported to the new API, python-geoclue will be killed as presumably there's nothing using it and presumably the new version will be introspection enabled. For redshift and vfrnav I doubt it works properly so I'd just disable the geoclue support in those until they update to the newer API.
 
> We need to have these parallel installable for a time to be able to move
> forward with gnome-maps without having to wait on other apps getting ported
> first. I'd like to get gnome-maps (and geoclue2 and geocode-glib) in by the
> time of the F20 Alpha Freeze, which is on 2013-09-03, so that we can ship it
> on the Desktop live media.

I would just kill the support for the old version in general and hence that would be less work as in most cases geoclue never really worked effectively anyway so I just don't see the need to keep it around at all.
Comment 12 Kalev Lember 2013-08-23 06:59:49 EDT
(In reply to Peter Robinson from comment #11)
> Of the above the presumably webkit/empathy will be ported to the new API

Sure, eventually. And hopefully soon so we can kill off the old geoclue package. But not in a week's time that's left for F20 Alpha.
Comment 13 Peter Robinson 2013-08-23 07:02:33 EDT
(In reply to Kalev Lember from comment #12)
> (In reply to Peter Robinson from comment #11)
> > Of the above the presumably webkit/empathy will be ported to the new API
> 
> Sure, eventually. And hopefully soon so we can kill off the old geoclue
> package. But not in a week's time that's left for F20 Alpha.

If it doesn't work in those packages I think we'd be better off disabling the functionality until it's ported and does work and in that case it would be easily doable in that time.
Comment 14 Kalev Lember 2013-08-23 13:43:43 EDT
* Fri Aug 23 2013 Kalev Lember <kalevlember@gmail.com> - 1.99.2-1
- Update to 1.99.2
- The shared library is gone in this release and all users should use the
  dbus service directly

Spec URL: http://kalev.fedorapeople.org/geoclue2.spec
SRPM URL: http://kalev.fedorapeople.org/geoclue2-1.99.2-1.fc20.src.rpm
Comment 15 Baptiste Mille-Mathias 2013-08-24 10:07:40 EDT
Hello,

Here's my manual review based on the item from the page http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
=============================================================================
=== MUST ===
MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.
 rpmlint geoclue2-1.99.1-3.fc20.src.rpm
 geoclue2.src: W: spelling-error Summary(en_US) Geolocation -> Geo location, Geo-location, Echolocation
 geoclue2.src: W: spelling-error %description -l en_US Geoclue -> Geo clue, Geo-clue, Occlude
 	spelling-error: false positive

 rpmlint geoclue2-1.99.2-1.fc20.x86_64.rpm
 geoclue2.x86_64: W: spelling-error Summary(en_US) Geolocation -> Geo location, Geo-location, Echolocation
 geoclue2.x86_64: W: spelling-error %description -l en_US Geoclue -> Geo clue, Geo-clue, Occlude
 geoclue2.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/org.freedesktop.GeoClue2.conf
 	spelling-error: false positive
 	non-conffile-in-etc: seems to be a false positive. (worth to report a bug to rpmlint ?)
 
 rpmlint geoclue2-devel-1.99.2-1.fc20.x86_64.rpm 
 geoclue2-devel.x86_64: W: no-documentation
 	no documentation: should the README has to be shipped in this package too ?
 
 rpmlint geoclue2-server-1.99.2-1.fc20.x86_64.rpm 
 geoclue2-server.x86_64: W: spelling-error Summary(en_US) geolocation -> echolocation, collocation, allocation
 geoclue2-server.x86_64: W: spelling-error %description -l en_US geoip -> George
 geoclue2-server.x86_64: W: spelling-error %description -l en_US lookup -> lockup, hookup, look up
 geoclue2-server.x86_64: W: spelling-error %description -l en_US geolocation -> echolocation, collocation, allocation
 geoclue2-server.x86_64: W: no-documentation
 geoclue2-server.x86_64: W: no-manual-page-for-binary geoip-update
 geoclue2-server.x86_64: W: no-manual-page-for-binary geoip-lookup

 	spelling-error: false positive
 	no documentation: should the README has to be shipped in this package too ?
	no-manual-page-for-binary: a bug tracker should be open with upstream to have a man page. I don't know if it's a show stopper though
	pass: some false positive, open a bug for man pages

MUST: The package must be named according to the Package Naming Guidelines .
	name OK, faily simple, not specific case.
	won't clash with package geoclue

MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
	pass

MUST: The package must meet the Packaging Guidelines .


MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
	pass (GPLv2+) licencecheck done

MUST: The License field in the package spec file must match the actual license.
	pass, GPLv2+ found on each header

MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
	pass, file COPYING included

MUST: The spec file must be written in American English.
	pass

MUST: The spec file for the package MUST be legible.
	pass

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use sha256sum for this task as it is used by the sources file once imported into git. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
     pass hash is the same (bbde33254bf35a13b1982f5357a0df61e73111aba8e0d799e17e2a9108754390)

MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
	Pass on F20 target for all archs
	on F19, build fails due to geoip being too old.

MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line.
	pass: all builds submitted on koji ran succesfully

MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
	pass: All build dependencies listed
	Shouldn't the minimal version be specified ?

MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
	N/A

MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
	pass: ldconfig called

MUST: Packages must NOT bundle copies of system libraries.
	pass: N/A

MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker
	pass: N/A

MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
	pass: there is no dependency on another package directory

MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)
	pass: No file listed more than once

MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. 
	pass: no problem found, all binaries have x bit set 

MUST: Each package must consistently use macros.
	pass: consistent use of macro

MUST: The package must contain code, or permissable content.
	pass: Code only with licence well defined

MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity).
	pass: N/A

MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present.
	pass: No dependency on %doc 

MUST: Static libraries must be in a -static package.
	pass: N/A

MUST: Development files must be in a -devel package.
	pass: geoclue.pc provided in -devel

MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release}
	pass: done for devel and for server to depends on the same arch/version of package geoclue

MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
	Pass: *.la removed

MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation.
	pass: N/A

MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time.
	pass: no problem found there.

MUST: All filenames in rpm packages must be valid UTF-8.
	pass: ok

=== SHOULD ===
SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
	pass: N/A licence provided

SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. 
	pass: N/A, no translation

SHOULD: The reviewer should test that the package builds in mock.
	pass: build on koji

SHOULD: The package should compile and build into binary rpms on all supported architectures.
	pass: build on x86/x64/arm

SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
	pass: N/A don't own a F20 machine

SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.
	pass: N/A

SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
	pass: Done

SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb.
	pass: Done

SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
	pass: N/A

SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.
	pass: Already reported above
=============================================================================
Package seems to me to be good except some corrections listed above, however Haïkel has to validate my words.

Regards
Comment 16 Elad Alfassa 2013-08-24 10:17:15 EDT
Shipping the README is not mandatory, especially if it doesn't contain any useful information.

Mind if I proceed with a formal review?
Comment 17 Kalev Lember 2013-08-24 10:41:04 EDT
(In reply to Baptiste Mille-Mathias from comment #15)
> Here's my manual review based on the item from the page
> http://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Good review! I think it makes sense to do it manually the first time; later when you already know what to check for, fedora-review can make it much quicker.

Some comments inline:


>  geoclue2-server.x86_64: W: no-documentation
>  geoclue2-server.x86_64: W: no-manual-page-for-binary geoip-update
>  geoclue2-server.x86_64: W: no-manual-page-for-binary geoip-lookup
> 
>  	spelling-error: false positive
>  	no documentation: should the README has to be shipped in this package too ?

rpmlint warnings are just warnings, not blockers. It's basically saying 'Hey dude, can you double check that you didn't forget this thing?' (Also, rpmlint upstream is not controlled by Fedora, so sometimes it gives errors about items that are perfectly fine in Fedora; at other times it misses important stuff.)

In this case, the I think it would make sense to include src/geoip-server/API-Documentation.txt as it includes instructions how to set up the server. I've done that now.

README doesn't really say much; it only has the description of the package and that's already part of %description. We might want to add it at a later date if it gets some more content, but now it would just duplicate the rpm description.


> MUST: The sources used to build the package must match the upstream source,
> as provided in the spec URL. Reviewers should use sha256sum for this task as
> it is used by the sources file once imported into git.

The review guidelines are actually wrong here because the lookaside cache still uses md5sum :) Anyway, you've verified that the sources match and that's what matters.

> If no upstream URL
> can be specified for this package, please see the Source URL Guidelines for
> how to deal with this.
>      pass hash is the same
> (bbde33254bf35a13b1982f5357a0df61e73111aba8e0d799e17e2a9108754390)


> MUST: The package MUST successfully compile and build into binary rpms on at
> least one primary architecture.
> 	Pass on F20 target for all archs
> 	on F19, build fails due to geoip being too old.

That's fine, we only need it for F20+.


> MUST: All build dependencies must be listed in BuildRequires, except for any
> that are listed in the exceptions section of the Packaging Guidelines ;
> inclusion of those as BuildRequires is optional. Apply common sense.
> 	pass: All build dependencies listed
> 	Shouldn't the minimal version be specified ?

Some people like to specify the BR versions; others don't. My personal opinion is that we're building a binary distro and as such it's important to specify tight Requires for binary packages so that the end users install pull in correct dependencies. For the source packages (BuildRequires), it's less important because we know what's in koji; I already know that the package versions that are in F20 are high enough for this package.


> MUST: Every binary RPM package (or subpackage) which stores shared library
> files (not just symlinks) in any of the dynamic linker's default paths, must
> call ldconfig in %post and %postun.
> 	pass: ldconfig called

This is actually a mistake on my part, I forgot to remove the ldconfig calls when we dropped the shared library in 1.99.2. Fixed now.
Comment 18 Kalev Lember 2013-08-24 10:48:32 EDT
* Sat Aug 24 2013 Kalev Lember <kalevlember@gmail.com> - 1.99.2-2
- Review fixes (#999153)
- Drop ldconfig calls that are unnecessary now that the shared library is gone
- Drop the build dep on gobject-introspection-devel
- Include API-Documentation.txt in the -server subpackage

Spec URL: http://kalev.fedorapeople.org/geoclue2.spec
SRPM URL: http://kalev.fedorapeople.org/geoclue2-1.99.2-2.fc20.src.rpm
Comment 19 Elad Alfassa 2013-08-24 10:54:51 EDT
If we don't ship a shared library, it makes no sense to ship a pkgconfig file
Comment 20 Kalev Lember 2013-08-24 10:58:14 EDT
Upstream is planning to use the pkgconfig file to tell other packages where the geoclue-interface.xml file is installed.
Comment 21 Michael Schwendt 2013-08-24 18:11:08 EDT
Indeed.  pkgconfig is not specific to libs and cflags. For example, "pkg-config --variable=NAME" queries are common. Also don't forget other checks pkgconfig can perform, such as --exists --atleast-version=…


> %description    devel
> The %{name}-devel package contains libraries and header files for
> developing applications that use %{name}.

That's not true anymore, if just the .pc file is included.
Comment 22 Kalev Lember 2013-08-24 18:35:32 EDT
Fixed, thanks Michael!


* Sat Aug 24 2013 Kalev Lember <kalevlember@gmail.com> - 1.99.2-3
- Update -devel subpackage description (#999153)

Spec URL: http://kalev.fedorapeople.org/geoclue2.spec
SRPM URL: http://kalev.fedorapeople.org/geoclue2-1.99.2-3.fc20.src.rpm
Comment 23 Haïkel Guémar 2013-08-25 10:01:34 EDT
Everything is fine but one thing:  shouldn't /etc/dbus-1/system.d/org.freedesktop.GeoClue2.conf be marked as %config ?
They are supposed to configure access to dbus API provided by this service, so in my opinion, they are configuration files and should be marked as such. But i won't consider this as a blocker if there are any arguments against it.
Comment 24 Zeeshan Ali 2013-08-25 10:24:05 EDT
(In reply to Haïkel Guémar from comment #23)
> Everything is fine but one thing:  shouldn't
> /etc/dbus-1/system.d/org.freedesktop.GeoClue2.conf be marked as %config ?
> They are supposed to configure access to dbus API provided by this service,
> so in my opinion, they are configuration files and should be marked as such.
> But i won't consider this as a blocker if there are any arguments against it.

Yeah, I think it should be. Better first check w/ other files in that directory and see if owning packages do the same, just to be sure though.
Comment 25 Haïkel Guémar 2013-08-25 10:40:59 EDT
@zeeshan: thanks for your answer.

A previous discussion on fedora devel mailing lists brought up by kalev on irc solves that matter: https://www.redhat.com/archives/fedora-packaging/2009-May/msg00065.html
Due to the consequences of editing these files, i can't consider this point as a blocker, i'd rather have this fixed upstream in DBus. As long as the packager is aware of that issue, i'm fine with it, either way, he choses.

Packaging guidelines should be updated to cover this exception (and we should ask/help upstream to fix this issue) or enforce the existing rule.

Since every other issues raised in this review have been solved (thanks to everyone who participated here), i hereby approve geoclue2 into Fedora Packages Collection.

@Baptiste: you did well for your first review :)


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
     Note: Using prebuilt rpms.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later)", "Unknown or generated". 2 files have unknown
     license. Detailed output of licensecheck in
     /home/haikel/999153-geoclue2/licensecheck.txt
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Large documentation must go in a -doc subpackage.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
     Note: Re-using old build in mock
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: No rpmlint messages.

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5851036
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
=> fixed in latest revision
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see attached
     diff).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
geoclue2.x86_64: I: checking
geoclue2.x86_64: W: spelling-error Summary(en_US) Geolocation -> Geo location, Geo-location, Echolocation
The value of this tag appears to be misspelled. Please double-check.

geoclue2.x86_64: W: spelling-error %description -l en_US Geoclue -> Geo clue, Geo-clue, Occlude
The value of this tag appears to be misspelled. Please double-check.

geoclue2.x86_64: I: checking-url http://www.freedesktop.org/wiki/Software/GeoClue/ (timeout 10 seconds)
geoclue2.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/org.freedesktop.GeoClue2.conf
A non-executable file in your package is being installed in /etc, but is not a
configuration file. All non-executable files in /etc should be configuration
files. Mark the file as %config in the spec file.

geoclue2-devel.x86_64: I: checking
geoclue2-devel.x86_64: I: checking-url http://www.freedesktop.org/wiki/Software/GeoClue/ (timeout 10 seconds)
geoclue2-devel.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

geoclue2-server.x86_64: I: checking
geoclue2-server.x86_64: W: spelling-error Summary(en_US) geolocation -> echolocation, collocation, allocation
The value of this tag appears to be misspelled. Please double-check.

geoclue2-server.x86_64: W: spelling-error %description -l en_US geoip -> George
The value of this tag appears to be misspelled. Please double-check.

geoclue2-server.x86_64: W: spelling-error %description -l en_US lookup -> lockup, hookup, look up
The value of this tag appears to be misspelled. Please double-check.

geoclue2-server.x86_64: W: spelling-error %description -l en_US geolocation -> echolocation, collocation, allocation
The value of this tag appears to be misspelled. Please double-check.

geoclue2-server.x86_64: I: checking-url http://www.freedesktop.org/wiki/Software/GeoClue/ (timeout 10 seconds)
geoclue2-server.x86_64: W: no-manual-page-for-binary geoip-update
Each executable in standard binary directories should have a man page.

geoclue2-server.x86_64: W: no-manual-page-for-binary geoip-lookup
Each executable in standard binary directories should have a man page.

3 packages and 0 specfiles checked; 0 errors, 10 warnings.


=> the /etc/dbus-1/system.d/org.freedesktop.GeoClue2.conf is not an issue as stated previously

Requires
--------


Provides
--------


Source checksums
----------------
http://people.freedesktop.org/~zeenix/releases/geoclue-1.99.2.tar.xz :
  CHECKSUM(SHA256) this package     : bbde33254bf35a13b1982f5357a0df61e73111aba8e0d799e17e2a9108754390
  CHECKSUM(SHA256) upstream package : bbde33254bf35a13b1982f5357a0df61e73111aba8e0d799e17e2a9108754390


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Comment 26 Kalev Lember 2013-08-25 10:55:27 EDT
Thanks for the review!

New Package SCM Request
=======================
Package Name: geoclue2
Short Description: Geolocation service
Owners: zeenix hadess kalev
Branches: f20
InitialCC:
Comment 27 Gwyn Ciesla 2013-08-26 08:26:17 EDT
Git done (by process-git-requests).
Comment 28 Kalev Lember 2013-08-26 09:48:24 EDT
Package imported and built; closing the ticket.

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