Bug 1126459 - Review Request: qqwing - library for sudoku generation and solving
Summary: Review Request: qqwing - library for sudoku generation and solving
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Kalev Lember
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-08-04 13:25 UTC by Michael Catanzaro
Modified: 2014-08-19 22:32 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-08-19 22:32:38 UTC
kalevlember: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Michael Catanzaro 2014-08-04 13:25:45 UTC
Spec URL: https://people.gnome.org/~mcatanzaro/qqwing/libqqwing.spec
SRPM URL: https://people.gnome.org/~mcatanzaro/qqwing/libqqwing-1.1.2-1.fc20.src.rpm
Description: libqqwing is a C++ library for solving and generating Sudoku puzzles
Fedora Account System Username: Catanzaro

Additional info: I'm the upstream maintainer of gnome-sudoku, which wants to depend on qqwing beginning with 3.14 (or maybe 3.16 if the dependency isn't approved upstream for 3.14).  I've also been successfully working with qqwing upstream to make a few changes.

Comment 1 Michael Catanzaro 2014-08-04 14:00:40 UTC
Whoops, that SPRM did not match that spec. Try #2:

https://people.gnome.org/~mcatanzaro/qqwing2/libqqwing.spec
https://people.gnome.org/~mcatanzaro/qqwing2/libqqwing-1.1.2-1.fc20.src.rpm

Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7239610

rpmlint says:

libqqwing.x86_64: E: incorrect-fsf-address /usr/share/doc/libqqwing/COPYING
libqqwing-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/qqwing-1.1.2/qqwing.cpp
libqqwing-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/qqwing-1.1.2/qqwing.hpp
libqqwing-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/qqwing-1.1.2/main.cpp
libqqwing-devel.x86_64: W: only-non-binary-in-usr-lib
libqqwing-devel.x86_64: W: no-documentation
libqqwing-devel.x86_64: E: incorrect-fsf-address /usr/include/qqwing.hpp
qqwing.x86_64: W: no-manual-page-for-binary qqwing
5 packages and 1 specfiles checked; 5 errors, 3 warnings.

I've added a man page and fixed the incorrect-fsf-address errors upstream. only-non-binary-in-usr-lib is complaining about the pkgconfig file. The devel package indeed has no documentation.

Comment 2 Michael Catanzaro 2014-08-04 14:08:38 UTC
Blocking FE-NEEDSPONSOR since I'm not a packager yet. Kalev has agreed to sponsor me.

Comment 3 Kalev Lember 2014-08-12 12:48:46 UTC
Hi Michael,

Thanks for packaging this up and sorry for the delay; I'm now back from GUADEC / Flock so lets try and get this in.

The spec file looks nice and clean, but I have a question about naming. Usually packages in Fedora follow the convention where upstream tarball name == srpm name. This often makes packaging simpler and easier to understand. Have you considered calling the spec/srpm "qqwing" and name the binary packages "qqwing", "qqwing-devel" and "qqwing-libs"?

It's definitely not a blocker and it's fine the way it is now too, just wondering if you had any special reason for doing a slightly different thing here.

Comment 4 Michael Catanzaro 2014-08-13 00:36:01 UTC
Since qqwing (the executable) depends on libqqwing, I thought it might be odd that the main package depended on the subpackage; with libqqwing as the main package, the subpackage depends on the main package. That was my only reason. If it's OK for the main package to require the subpackage, then I'll change it so that qqwing is the base package and libqqwing the subpackage.

Comment 5 Kalev Lember 2014-08-13 16:13:31 UTC
Having the main package depend on a subpackage is very common. Nothing wrong with doing that.

Just a random example that does this: http://pkgs.fedoraproject.org/cgit/evince.git/tree/evince.spec

Comment 6 Michael Catanzaro 2014-08-15 13:34:31 UTC
OK, updated to make libqqwing the subpackage, and also use the latest upstream release:

https://people.gnome.org/~mcatanzaro/qqwing3/qqwing.spec
https://people.gnome.org/~mcatanzaro/qqwing3/qqwing-1.1.3-1.fc20.src.rpm

Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7310772

libqqwing-devel.x86_64: W: only-non-binary-in-usr-lib
libqqwing-devel.x86_64: W: no-documentation
5 packages and 1 specfiles checked; 0 errors, 2 warnings.

Comment 7 Michael Catanzaro 2014-08-15 13:44:39 UTC
One more try, using the idiomatic package names you suggested this time:

https://people.gnome.org/~mcatanzaro/qqwing4/qqwing.spec
https://people.gnome.org/~mcatanzaro/qqwing4/qqwing-1.1.3-1.fc20.src.rpm

Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7310808

qqwing-devel.x86_64: W: spelling-error Summary(en_US) libqqwing -> liberating
qqwing-devel.x86_64: W: spelling-error %description -l en_US libqqwing -> liberating
qqwing-devel.x86_64: W: only-non-binary-in-usr-lib
qqwing-devel.x86_64: W: no-documentation
qqwing-libs.x86_64: W: spelling-error %description -l en_US libqqwing -> liberating
5 packages and 1 specfiles checked; 0 errors, 5 warnings.

Comment 8 Kalev Lember 2014-08-18 20:10:35 UTC
This looks great to me, thanks!


Two very minor nits:

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

The description needs updating after the libqqwing -> qqwing rename. Could use the %{name} macro here since it semantically refers to the base package's name.

> %{_mandir}/man1/qqwing.1.gz

Would be better to use a glob here to make sure this line works when the man page compression used by rpmbuild gets changed. Something like:

%{_mandir}/man1/qqwing.1*

Comment 9 Kalev Lember 2014-08-18 20:22:05 UTC
Fedora review qqwing-1.1.3-1.fc20.src.rpm 2014-08-18

+ OK
! needs attention

+ rpmlint warnings 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 the sources in the srpm
  a3822456e01f74db6e7a2e5ae135ad78  qqwing-1.1.3.tar.gz
  a3822456e01f74db6e7a2e5ae135ad78  Download/qqwing-1.1.3.tar.gz
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a locale handling
+ 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
+ Header files should be in -devel
n/a Static libraries should be in -static
+ Library files that end in .so must go in a -devel package
+ -devel must require the fully versioned base
+ Packages should not contain libtool .la files
n/a Proper .desktop file handling
+ Doesn't own files or directories already owned by other packages
+ Filenames are valid UTF-8


APPROVED

Comment 10 Kalev Lember 2014-08-18 20:42:19 UTC
The package looks good and feel free to fix up the minor issues I pointed out in comment #8 when commiting this to git; no need to upload the spec / srpm files for another review round. Or you can, of course, if you want to :-)

Next thing would be sponsoring you to the packager group so you can request the git repo creation.

One thing that I usually ask new packagers to do before sponsoring them is to go through some other review request and try to do an official review there. Every packager is also automatically a reviewer and it's important to know how to review other people's packages. For a review, I usually just go through the review checklist in https://fedoraproject.org/wiki/Packaging:ReviewGuidelines and post a short comment for each MUST line, indicating whether it passes or not. There's a helper called fedora-review to automate this, but it can be instructional to go through this manually first.

Anyway, I don't want to delay this package any more, since it's the 3.13.90 tarball date today. Would be great if you could look into reviewing something (maybe GNOME-related?) one day.

So, welcome to Fedora packaging Michael! Glad to have you. I've sponsored you now to the packager group; the permissions might take up to an hour to sync everywhere. Would be good if you could join the announce lists (announce and devel-announce).

Feel free to look me up anytime, either on IRC or by email. Happy to help find answers to any packaging related questions.

Comment 11 Michael Catanzaro 2014-08-19 14:42:31 UTC
New Package SCM Request
=======================
Package Name: qqwing
Short Description: Sudoku solver and generator
Upstream URL: http://ostermiller.org/qqwing/
Owners: Catanzaro
Branches: f21 f22
InitialCC:

Comment 12 Gwyn Ciesla 2014-08-19 15:42:55 UTC
WARNING: "Catanzaro" is not a valid FAS account.
WARNING: Invalid branch f22 requested 

f22 is master/devel and is automatic.

Comment 13 Michael Catanzaro 2014-08-19 15:50:24 UTC
Thanks Jon, sorry about that!

Comment 14 Michael Catanzaro 2014-08-19 15:50:46 UTC
New Package SCM Request
=======================
Package Name: qqwing
Short Description: Sudoku solver and generator
Upstream URL: http://ostermiller.org/qqwing/
Owners: catanzaro
Branches: f21
InitialCC:

Comment 15 Gwyn Ciesla 2014-08-19 19:45:50 UTC
Git done (by process-git-requests).

Comment 16 Michael Catanzaro 2014-08-19 22:32:38 UTC
OK, qqwing is built for F21 and F22, so you should be good to go on GNOME Sudoku.


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