Bug 670127 - Review Request: the-board - A space for placing daily records in your GNOME desktop
Summary: Review Request: the-board - A space for placing daily records in your GNOME d...
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Bill Nottingham
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-01-17 09:49 UTC by Cosimo Cecchi
Modified: 2014-03-17 03:26 UTC (History)
10 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2011-05-07 19:15:23 UTC
notting: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Cosimo Cecchi 2011-01-17 09:49:54 UTC
Spec URL: http://people.gnome.org/~cosimoc/the-board-pkg/the-board.spec
SRPM URL: http://people.gnome.org/~cosimoc/the-board-pkg/the-board-0.1.0-1.fc15.src.rpm
Description: A space for placing daily records in your GNOME desktop, see [1]

[1] http://live.gnome.org/TheBoardProject

Note that the-board depends on a libmx version newer than what's currently in Rawhide, see https://bugzilla.redhat.com/show_bug.cgi?id=670120 about this.

Comment 1 Fabian Affolter 2011-01-17 16:11:29 UTC
Just some quick comments:

- You are mixing %{buildroot} and $RPM_BUILD_ROOT
- All doc files (README, NEWS, etc) should go to %doc
- 'desktop-file-install' is not used https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files
- Isn't RPM picking the gobject-introspection dependency automatically?
- The icon cache update is missing. https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

Comment 2 Cosimo Cecchi 2011-01-18 14:06:46 UTC
Thanks for the review Fabian.

I addressed your comments and suppressed some more warnings from rpmlint, whose output looks pretty OK now.

New version
SPEC URL: http://people.gnome.org/~cosimoc/the-board-pkg/the-board.spec
SRPM URL: http://people.gnome.org/~cosimoc/the-board-pkg/the-board-0.1.0-2.fc15.src.rpm

Comment 3 Matthias Clasen 2011-01-20 19:44:43 UTC
You don't need to rm -rf %{buildroot} in %install anymore, and %clean can go altogether.

Comment 4 Cosimo Cecchi 2011-01-21 13:38:03 UTC
(In reply to comment #3)
> You don't need to rm -rf %{buildroot} in %install anymore, and %clean can go
> altogether.

Ok, new versions addressing Matthias' review.

SPEC: http://people.gnome.org/~cosimoc/the-board-pkg/the-board.spec
SRPM: http://people.gnome.org/~cosimoc/the-board-pkg/the-board-0.1.0-3.fc15.src.rpm

Comment 5 Matthias Clasen 2011-01-21 15:35:05 UTC
Builds fine in mock after adding libmx-1.1 to a local repo

rpmlint output:

rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/the-board-*.rpm
the-board.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/the-board-start ['/usr/lib64']
the-board.x86_64: E: zero-length /usr/share/doc/the-board-0.1.0/AUTHORS
the-board.x86_64: W: no-manual-page-for-binary the-board
the-board-devel.x86_64: W: no-documentation
the-board-nautilus.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 2 errors, 3 warnings.

the errors should be addressed, the warnings are harmless.

Comment 6 Cosimo Cecchi 2011-01-28 17:10:23 UTC
(In reply to comment #5)

> the errors should be addressed, the warnings are harmless.

New SPEC at the same place: http://people.gnome.org/~cosimoc/the-board-pkg/the-board.spec
New SRPM: http://people.gnome.org/~cosimoc/the-board-pkg/the-board-0.1.0-4.fc15.src.rpm

But still no new libmx in rawhide

Comment 7 Matthias Clasen 2011-01-28 18:29:08 UTC
I still get

$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
the-board.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/the-board-start ['/usr/lib64']
the-board.x86_64: W: no-manual-page-for-binary the-board
the-board-devel.x86_64: W: no-documentation
the-board-nautilus.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 1 errors, 3 warnings.

so I guess --disable-rpath is not good enough

Comment 8 Colin Walters 2011-01-28 19:11:01 UTC
One complicating issue is if the-board uses gobject-introspection, the rpath is necessary for the dumper to work correctly.  You'll have to use chrpath --delete after the build is complete.

Comment 9 Cosimo Cecchi 2011-01-28 20:48:20 UTC
(In reply to comment #8)
> One complicating issue is if the-board uses gobject-introspection, the rpath is
> necessary for the dumper to work correctly.  You'll have to use chrpath
> --delete after the build is complete.

I tried this method, and setup a mock build myself to verify if it was working correctly this time, and it seems it did.

New SPEC at the same place:
http://people.gnome.org/~cosimoc/the-board-pkg/the-board.spec
New SRPM:
http://people.gnome.org/~cosimoc/the-board-pkg/the-board-0.1.0-5.fc15.src.rpm

Also, a newer libmx hit rawhide today.

Comment 10 Matthias Clasen 2011-02-01 01:35:15 UTC
ok, going down the checklist now:

rpmlint: see above, ok
package name: ok
spec file name: ok
packaging guidelines: ok, except as noted below
license: ok
license field: ok
license file: ok
spec language: ok
spec readable: ok
upstream sources: ok
buildable: ok
excludearch: ok
buildrequires: ok
locale handling: ok
ldconfig: need to run /sbin/ldconfig in %post/%postun
system libraries: ok, although libtb is a pretty generic name for an
  app-specific library
relocatable: ok
directory ownership: I think the -nautilus package should perhaps require 
  nautilus, instead of owning %libdir/nautilus ? at least that's what other
  -nautilus subpackages do
duplicate files: ok
file permissions: ok
macro use: ok
permissible content: ok
large docs: ok
%doc content: ok
headers: ok
static libs: ok
shared lib symlinks: ok
devel dep: ok
libtool archives: ok
desktop file: ok
duplicate ownership: ok
utf8 filenames: ok

I don't think the-board should ship its own fonts.conf, in particular not one that contains a reference to /home/lucasr/Code...

Comment 11 Christoph Wickert 2011-02-23 11:54:26 UTC
Please use %global instead of %define, see 
https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define

Comment 12 Christoph Wickert 2011-03-23 09:19:07 UTC
Peter, midori doesn't like bugzilla and changes the component to the first in the list. Changing it back to 'Package review'.

Comment 13 Cosimo Cecchi 2011-03-28 13:27:55 UTC
Ok, I gave this another pass, and upadted the package to the latest 0.1.1.1 too.

SPEC: http://people.gnome.org/~cosimoc/the-board-pkg/the-board.spec
SRPM: http://people.gnome.org/~cosimoc/the-board-pkg/the-board-0.1.1.1-1.fc15.src.rpm

Comment 14 Cosimo Cecchi 2011-03-29 00:19:40 UTC
Today upstream released a new version, 0.1.2, so I updated the package again.

SPEC: http://people.gnome.org/~cosimoc/the-board-pkg/the-board.spec
SRPM: http://people.gnome.org/~cosimoc/the-board-pkg/the-board-0.1.2-1.fc15.src.rpm

Comment 15 Bill Nottingham 2011-03-29 02:04:15 UTC
So, looking at the updated version (review appears to have stalled)?:

- Package meets naming and packaging guidelines - OK
- Spec file matches base package name. - OK
- Spec has consistant macro usage.  - OK
- Meets Packaging Guidelines. - OK
- License - ***

The license is declared as GPLv3, and that's what's included in the tarball.

However, the only source file with a license is LGPLv2+.

- License field in spec matches - OK (matches COPYING)
- License file included in package - OK
- Spec in American English - OK
- Spec is legible - OK.
- Sources match upstream md5sum:
284162a6a1a6b7762a89b021833a12835a334e535061626269096fb8a96c7e78  /home/notting/prog/rpm/source/the-board-0.1.2.tar.bz2

OK

- Package needs ExcludeArch - N/A
- BuildRequires correct - OK
- Spec handles locales/find_lang - OK
- Package is relocatable and has a reason to be. - N/A
- Package has %defattr and permissions on files is good. - OK
- Package is code or permissible content. - OK
- Doc subpackage needed/used. - N/A
- Packages %doc files don't affect runtime. - OK

- Headers/static libs in -devel subpackage.  - OK
- Spec has needed ldconfig in post and postun - OK
- .pc files in -devel subpackage/requires pkgconfig - N/A
- .so files in -devel subpackage. - OK
- -devel package Requires: %{name} = %{version}-%{release} - OK
- .la files are removed. - OK

- Package is a GUI app and has a .desktop file - OK

- Package compiles and builds on at least one arch. - OK (tested x86_64)
- Package has no duplicate files in %files. - OK
- Package doesn't own any directories other packages own. - OK
- Package owns all the directories it creates. - OK
- No rpmlint output:

the-board.x86_64: W: no-manual-page-for-binary the-board
the-board-devel.x86_64: W: no-documentation
the-board-devel.x86_64: W: no-manual-page-for-binary tb-js-unit
the-board-nautilus.x86_64: W: no-documentation

These are all upstream issues, so OK.

- final provides and requires are sane - OK

SHOULD Items:

- Should build in mock. - OK (tested x86_64)
- Should function as described. - OK
- Should have sane scriptlets. - OK
- Should have subpackages require base package with fully versioned depend. - OK
- Should have dist tag - OK
- Should package latest version - OK

So, the only issue I can see is the license mismatch. Probably just requires a clarification from upstream that GPLv3 is their intent?

Comment 16 Lucas Rocha 2011-03-29 15:31:45 UTC
Hi guys, GPLv3 is my intent. The reason TbBox is licensed LGPLv2+ is because I copied it from litl's LGPLv2+-licensed code which was based on Red Hat's Hippo Canvas. To be honest, I'm not sure what is the legally correct thing to do here. In theory, I can just relicense the code "under any version of the GPL since GPLv2"[1]. Not sure it matters much.

Thanks!

[1] http://www.gnu.org/licenses/gpl-faq.html#AllCompatibility

Comment 17 Bill Nottingham 2011-03-30 15:40:24 UTC
OK, that works for me. Lucas - if you want to slap a GPLv3 copyright boilerplate on some of the other source files, it would make the situation cleaner for future releases.

In any case, approved.

Comment 18 Cosimo Cecchi 2011-03-30 23:46:25 UTC
New Package SCM Request
=======================
Package Name: the-board
Short Description: A space for placing daily records in your GNOME desktop
Owners: cosimoc
Branches: f15
InitialCC: cosimoc

Comment 19 Jason Tibbitts 2011-03-31 00:52:38 UTC
Git done (by process-git-requests).

Comment 20 Peter Robinson 2011-05-07 19:15:23 UTC
In rawhide and F-15


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