Bug 919265

Summary: Review Request: bijiben - Simple Note Viewer
Product: [Fedora] Fedora Reporter: Pierre-Yves Luyten <py>
Component: Package ReviewAssignee: Kalev Lember <kalevlember>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: amigadave, bochecha, dgsiegel, kalevlember, mclasen, michael.monreal+bugs, notting, package-review, py, samuel-rhbugs, sanjay.ankur, tcallawa
Target Milestone: ---Flags: kalevlember: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-05-09 20:44:58 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description Pierre-Yves Luyten 2013-03-07 18:59:14 EST
Spec URL: http://py.luyten.fr/Publique/gnome/bjb/3790/bijiben.spec
SRPM URL: http://py.luyten.fr/Publique/gnome/bjb/3790/bijiben-3.7.90-1.fc18.src.rpm
Description: Bijiben is a note editor designed to remain simple to use

Hi,

https://live.gnome.org/Design/Apps/Notes offered a nice design; Bijiben is an ongoing implementation in c (gtk / webkit).

I'm current maintainer of the application and put 3.7.90 spec until I submit current one - which needs more recent gtk. It would be my first fedora package,  I'll look for sponsor.
Comment 1 Pierre-Yves Luyten 2013-03-09 20:14:37 EST
I just built 3.7.91 and discovered some issues on previous spec. It's built from rawhide to have all the dependencies.

Spec: http://py.luyten.fr/Publique/gnome/bjb/3791/bijiben.spec
SRPM: http://py.luyten.fr/Publique/gnome/bjb/3791/bijiben-3.7.91-1.fc19.src.rpm
Comment 2 Mathieu Bridon 2013-03-13 04:15:41 EDT
I'm not a sponsor, so I can't accept your package, but I want to use your application in Fedora so I'll start the review, hopefully to speed up the inclusion. :)

There are many problems with your package, but most of them are trivial to fix.

I am a bit perplexed by the licensing of Bijiben, so I'm blocking FE-LEGAL, but I don't think there is any major problem, I'm just not sure what value to use for the License tag.

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

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


Issues:
=======
[!]: gtk-update-icon-cache is not invoked
     => See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

[!]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
     => See: http://fedoraproject.org/wiki/Packaging/Guidelines#desktop

     => The desktop file is installed by the Makefile, so you could use
          %install
          [... snip ...]
          desktop-file-validate %{buildroot}/%{_datadir}/applications/%{name}.desktop

[!]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     => See details below

[!]: Package contains no bundled libraries without FPC exception.
     => Package bundles libgd. I believe this is fine given the nature of
        libgd, but you must add:
            Provides: bundled(libgd)

[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     => I'm fairly confident that you will not build this package in an old
        EPEL, so please remove the clean section.

[!]: Package uses nothing in %doc for runtime.
     => Shouldn't %{_datadir}/help/C/%{name} be marked as %doc?

[!]: The spec file handles locales properly.
     => See https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files

[!]: Package must own all directories that it creates.
     => Package drops a file in %{_datadir}/gnome-shell/search-providers/
        This folder is owned by gnome-shell, but adding a requirement on it
        would be bad for people who want to use the application in other
        desktops. Please have bijiben own the folder.

     => Package drops a file in %{_libdir}/%{name}. Please own this folder.

[!]: Uses parallel make.
     => Use make %{?_smp_mflags}

[!]: Spec use %global instead of %define.
     Note: %define url_ver %(echo %{version}|cut -d. -f1,2)
     => Use %global instead

[!]: Final provides and requires are sane (see attachments).
     => Looking at gnome-photos, it neither provides not requires 'libgd.so()'
        It also doesn't even install it at all. Shouldn't you be doing the
        same thing?

[!]: Packages should try to preserve timestamps of original installed files.
     => Please pass INSTALL="/usr/bin/install -p" to the make install command


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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[!]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
     => See details below

[x]: %build honors applicable compiler flags or justifies otherwise.
[!]: Package contains no bundled libraries without FPC exception.
     => Package bundles libgd. I believe this is fine given the nature of
        libgd, but you must add:
            Provides: bundled(libgd)

[x]: Changelog in prescribed format.
     => Note that many people prefer leaving an empty line between changelog
        entries, as it makes the whole thing more readable. This is purely a
        matter of preference though, not a blocker.

[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     => I'm fairly confident that you will not build this package in an old
        EPEL, so please remove this line.

[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[!]: Package uses nothing in %doc for runtime.
     => Shouldn't %{_datadir}/help/C/%{name} be marked as %doc?

[x]: Package is not known to require ExcludeArch.
[x]: glib-compile-schemas is run if required
[!]: Package complies to the Packaging Guidelines
[!]: License field in the package spec file matches the actual license.
     => Bijiben seems to be a bit of a mix and match of licenses:
        - GPLv3+
          ./src/bjb-app-menu.c:
          ./src/bjb-controller.h:
          ./src/bjb-controller.c:
          ./src/bjb-note-view.c:
          ./src/bjb-bijiben.c:
          ./src/bjb-note-tag-dialog.c:
          ./src/bjb-main-toolbar.h:
          ./src/bjb-color-button.h:
          ./src/bjb-share.h:
          ./src/bjb-share.c:
          ./src/bjb-main-toolbar.c:
          ./src/bjb-color-button.c:
          ./src/bjb-settings.c:
          ./src/bjb-main.c:
          ./src/bjb-bijiben.h:
          ./src/bjb-main-view.c:
          ./src/libbiji/biji-string.h:
          ./src/libbiji/biji-timeout.c:
          ./src/libbiji/libbiji.h:
          ./src/libbiji/biji-date-time.c:
          ./src/libbiji/biji-note-book.c:
          ./src/libbiji/biji-timeout.h:
          ./src/libbiji/biji-tracker.c:
          ./src/libbiji/biji-note-obj.h:
          ./src/libbiji/biji-date-time.h:
          ./src/libbiji/editor/biji-webkit-editor.h:
          ./src/libbiji/editor/biji-webkit-editor.c:
          ./src/libbiji/biji-note-obj.c:
          ./src/libbiji/deserializer/biji-lazy-deserializer.c:
          ./src/libbiji/deserializer/biji-lazy-deserializer.h:
          ./src/libbiji/biji-string.c:
          ./src/libbiji/biji-tracker.h:
          ./src/libbiji/biji-note-id.c:
          ./src/libbiji/serializer/biji-lazy-serializer.c:
          ./src/libbiji/serializer/biji-lazy-serializer.h:
          ./src/libbiji/biji-note-id.h:
          ./src/libbiji/biji-zeitgeist.h:
          ./src/libbiji/biji-zeitgeist.c:
          ./src/bjb-note-tag-dialog.h:

        - LGPLv2 or LGPLv3
          ./src/libbiji/editor/biji-editor-utils.h:
          ./src/libbiji/editor/biji-editor-utils.c:
          ./src/libbiji/editor/biji-editor-selection.h:
          ./src/libbiji/editor/biji-editor-selection.c:
             => These don't attribute copyright to anybody. They seem to be 
                 coming from Evolution?

        - GPLv2+
          ./src/bjb-search-toolbar.c
              => This doesn't attribute copyright to anybody, is that a wrong
                 copy-paste?

          ./src/bjb-selection-toolbar.c:
          ./src/bjb-editor-toolbar.c:
          ./src/bijiben-shell-search-provider.c
              => These attribute copyright to Red Hat but not you, is that a
                 wrong copy-paste?

          ./src/bjb-selection-toolbar.h
          ./src/bjb-editor-toolbar.h
              => These talk about GNOME Photos and attribute copyright to
                 Red Hat but not you, is that a wrong copy-paste?

        - LGPLv2+
          ./libgd/*

     => The result of all this would be something like:
            GPLv3+ and (LGPLv2 or LGPLv3) GPLv2+ and LGPLv2+

        But to be honest, I have no idea what happens with such a complicated
        resulting license, so I'm blocking FE-LEGAL :-/

[!]: The spec file handles locales properly.
     => See https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files

[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.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[!]: Package must own all directories that it creates.
     => Package drops a file in %{_datadir}/gnome-shell/search-providers/
        This folder is owned by gnome-shell, but adding a requirement on it
        would be bad for people who want to use the application in other
        desktops. Please have bijiben own the folder.

     => Package drops a file in %{_libdir}/%{name}. Please own this folder.

[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]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 194560 bytes in 5 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[-]: Each %files section contains %defattr if rpm < 4.4
[-]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[-]: Fully versioned dependency in subpackages, if present.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[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.
[-]: 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]: 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.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.

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

Generic:
[!]: Uses parallel make.
     => Use make %{?_smp_mflags}

[!]: Spec use %global instead of %define.
     Note: %define url_ver %(echo %{version}|cut -d. -f1,2)
     => Use %global

[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     => I'm fairly confident that you will not build this package in an old
        EPEL, so please remove the clean section.

[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[!]: Final provides and requires are sane (see attachments).
     => Looking at gnome-photos, it neither provides not requires 'libgd.so()'
        It also doesn't even install it at all. Shouldn't you be doing the
        same thing?

[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: 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.
[!]: Packages should try to preserve timestamps of original installed files.
     => Please pass INSTALL="/usr/bin/install -p" to the make install command

[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.

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

Generic:
[-]: 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).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: bijiben-3.7.91-1.fc19.x86_64.rpm
bijiben.x86_64: W: no-manual-page-for-binary bijiben
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

     => You're upstream, so you could consider adding a man page for bijiben,
        but feel free to ignore this warning, it's all but a review blocker.


Rpmlint (installed packages)
----------------------------
# rpmlint bijiben
bijiben.x86_64: W: no-manual-page-for-binary bijiben
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'

     => You're upstream, so you could consider adding a man page for bijiben,
        but feel free to ignore this warning, it's all but a review blocker.


Requires
--------
bijiben (rpmlib, GLIBC filtered):
    /bin/sh
    libX11.so.6()(64bit)
    libXcomposite.so.1()(64bit)
    libXdamage.so.1()(64bit)
    libXext.so.6()(64bit)
    libXfixes.so.3()(64bit)
    libXi.so.6()(64bit)
    libXrandr.so.2()(64bit)
    libatk-1.0.so.0()(64bit)
    libc.so.6()(64bit)
    libcairo-gobject.so.2()(64bit)
    libcairo.so.2()(64bit)
    libclutter-1.0.so.0()(64bit)
    libclutter-gtk-1.0.so.0()(64bit)
    libcogl-pango.so.12()(64bit)
    libcogl.so.12()(64bit)
    libgd.so()(64bit)
    libgdk-3.so.0()(64bit)
    libgdk_pixbuf-2.0.so.0()(64bit)
    libgio-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgmodule-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgtk-3.so.0()(64bit)
    libjavascriptcoregtk-3.0.so.0()(64bit)
    libjson-glib-1.0.so.0()(64bit)
    libm.so.6()(64bit)
    libpango-1.0.so.0()(64bit)
    libpangocairo-1.0.so.0()(64bit)
    libpthread.so.0()(64bit)
    libsoup-2.4.so.1()(64bit)
    libtracker-sparql-0.16.so.0()(64bit)
    libuuid.so.1()(64bit)
    libuuid.so.1(UUID_1.0)(64bit)
    libwebkitgtk-3.0.so.0()(64bit)
    libxml2.so.2()(64bit)
    libxml2.so.2(LIBXML2_2.4.30)(64bit)
    libxml2.so.2(LIBXML2_2.5.0)(64bit)
    libxml2.so.2(LIBXML2_2.5.2)(64bit)
    libxml2.so.2(LIBXML2_2.6.0)(64bit)
    libzeitgeist-1.0.so.1()(64bit)
    rtld(GNU_HASH)


Provides
--------
bijiben:
    bijiben
    bijiben(x86-64)
    libgd.so()(64bit)
    mimehandler(x-scheme-handler/note)


Unversioned so-files
--------------------
bijiben: /usr/lib64/bijiben/libgd.so


MD5-sum check
-------------
http://download.gnome.org/sources/bijiben/3.7/bijiben-3.7.91.tar.xz :
  CHECKSUM(SHA256) this package     : 6835fd76c74c3de363759767367a293b56c72f2fdb74dc1f88d50ec291dca545
  CHECKSUM(SHA256) upstream package : 6835fd76c74c3de363759767367a293b56c72f2fdb74dc1f88d50ec291dca545


Generated by fedora-review 0.4.0 (660ce56) last change: 2013-01-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -b 919265 -m fedora-rawhide-x86_64
Comment 3 Pierre-Yves Luyten 2013-03-13 05:56:19 EDT
Hello Matthieu, thanks much for the review (sorry there was all these little things..). I will submit soon new SPEC and SRPM soon for the technical parts.

Concerning the license : yes, some of the code comes from other applications. I believe i should keep different copyright holders / authors depending on where the initial code comes from. So it's not a wrong copy paste, but I can add me if that's better.

To simplify things
* for GPLv2+ i should just choose GPLv3+.
* for the "LGPLv2+ / [LGPLv2 or LGPLv3]" distinction :

=> first idea, keep files and have package choose LGPLv2
keeping libgd submodule as it is, LGPLv2+, seems important. Can't we choose LGPLv2 for the package without touching the source files?

=> other idea, change files & have LGPLv2+
For the files coming from evolution, I can ask the author to authorize moving files in libbiji to LGPLv2+, or I can rewrite things myself using LGPLv2+.
Comment 4 Mathieu Bridon 2013-03-13 07:53:23 EDT
(In reply to comment #3)
> Hello Matthieu,

Only one "t". ;)

> thanks much for the review (sorry there was all these little
> things..).

No worries, the purpose of the review is also to learn about these things. :)

> Concerning the license : yes, some of the code comes from other
> applications. I believe i should keep different copyright holders / authors
> depending on where the initial code comes from. So it's not a wrong copy
> paste, but I can add me if that's better.

I see. You probably add yourself to every file you have modified anyway.

> To simplify things
> * for GPLv2+ i should just choose GPLv3+.

Indeed, you can do that, which simplifies the resulting License tag.

> * for the "LGPLv2+ / [LGPLv2 or LGPLv3]" distinction :
> 
> => first idea, keep files and have package choose LGPLv2
> keeping libgd submodule as it is, LGPLv2+, seems important. Can't we choose
> LGPLv2 for the package without touching the source files?

libgd's license is LGPLv2+

The license of the files coming from Evolution is "LGPLv2 or LGPLv3".

The result of these two parts is (if I'm not mistaken) "LGPLv2+ and (LGPLv2 or LGPLv3)". It is under **both** these licenses ("and"), not "either or".

So the License tag for the total package would be:
    License: GPLv3+ and GPLv2+ and LGPLv2+ and (LGPLv2 or LGPLv3)

If you decide to make the GPLv2+ files into GPLv3+ (as mentioned above), then the License tag for the package becomes:
    License: GPLv3+ and LGPLv2+ and (LGPLv2 or LGPLv3)

I don't think you can get any simpler than that at the moment.

In any case, libgd will go away at some point in the future, so I wouldn't worry too much about it.

> => other idea, change files & have LGPLv2+
> For the files coming from evolution, I can ask the author to authorize
> moving files in libbiji to LGPLv2+, or I can rewrite things myself using
> LGPLv2+.

Maybe.

-----

All in all, the above is my interpretation, and I'd prefer having the legal folks confirm what is the appropriate License tag to use here.

One thing I might not have made clear: even if I'm right in my first comment and the License tag ends as complicated as I suggested, I don't think that it is a legal issue, as all these licenses are (I believe) perfectly compatible with each other.

My comment in the review was simply that the License tag you used (GPLv2+) is wrong, and it should be set to (I think) what I suggested above.

Unless I'm completely wrong on this and the legal folks say that there is a fundamental problem with these licenses, that very complex License tag would be perfectly acceptable. All I reported was that your current License tag doesn't match what is actually in the package. :)
Comment 5 Matthias Clasen 2013-03-13 08:17:02 EDT
Hey Mathieu, I can sponsor Pierre-Yves; thanks for getting the package review started anyway. If you want to continue that would be fantastic. I'll look over what you've found so far
Comment 6 Matthias Clasen 2013-03-13 08:20:16 EDT
Wrt to libgd - that is not actually a separately installable library, just a git module that is being shared by a number of new GNOME applications, while the code is getting ready for eventual gtk inclusion. In any case, bijiben should not install it, but link against it statically. Pierre-Yves: you should add static to your LIBGD incantation in configure.ac
Comment 7 Matthias Clasen 2013-03-13 08:22:33 EDT
wrt:
     => Shouldn't %{_datadir}/help/C/%{name} be marked as %doc?

%find_lang does this for you if you use --with-gnome
Comment 8 Pierre-Yves Luyten 2013-03-14 21:09:57 EDT
Cool, %find_lang is now a good friend! Two patch were pushed (upstream so for 3.7.92 rc).

- Patch to get rid of GPLv2+
- Patch to use static link to libgd
- Use find_lang
- Remove define for url_ver
- desktop-file-validate
- update timestamp for icons
- BuildRequire gettext
- BuildRequire yelp-tools
- Provides bundled libgd
- Don't "clean"

SPEC: http://py.luyten.fr/Publique/gnome/bjb/3791-2/bijiben.spec
SRPM: http://py.luyten.fr/Publique/gnome/bjb/3791-2/bijiben-3.7.91-2.fc19.src.rpm
PATCH0: http://py.luyten.fr/Publique/gnome/bjb/3791-2/license-use-consistently-GPLv3.patch
PATCH1 : http://py.luyten.fr/Publique/gnome/bjb/3791-2/static-libgd.patch
Comment 9 Mathieu Bridon 2013-03-15 01:05:24 EDT
Thanks for the quick update Pierre-Yves, this is looking much better already!


Remaining issues:
================

[!]: gtk-update-icon-cache is not invoked
     => See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
     => You're missing the "/bin/touch --no-create ..." line in %post
     

[!]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
     => The license tag appears correct to me, but it is more complex than what I've
        been used to, so I'd appreciate a confirmation from the legal team.

[!]: Package fails to build in mock because of missing:
         BuildRequires: desktop-file-utils

Note, you should try to use mock to build your packages, as it would have caught this last problem for you. :)


Details:
========

--- 919265-bijiben.bak/srpm-unpacked/bijiben.spec	2013-03-13 12:26:50.367149275 +0800
+++ 919265-bijiben/srpm-unpacked/bijiben.spec	2013-03-15 11:34:06.642803119 +0800
@@ -1,14 +1,25 @@
-%define url_ver	%(echo %{version}|cut -d. -f1,2)

     => Heh, dropping the macro definition is another way of fixing the issue. :)

 Name:		bijiben
 Version:	3.7.91
-Release:	1%{?dist}
+Release:	2%{?dist}
 Summary:	Simple Note Viewer
-License:	GPLv2+
+
+# Bijiben is GPLv3+ appart a few files "LGPLv2 or LGPLv3"

     => Pedantic typo fix: "apart"

+# And ligd is LGPLv2+
+License:	GPLv3+ and LGPLv3 and LGPLv2+

     => The license tag appears correct to me, but it is more complex than what I've
        been used to, so I'd appreciate a confirmation from the legal team.

 Url:		http://www.gnome.org
-Source0:	http://download.gnome.org/sources/%{name}/%{url_ver}/%{name}-%{version}.tar.xz
+Source0:	http://ftp.gnome.org/pub/GNOME/sources/%{name}/3.7/%{name}-%{version}.tar.xz
+
+# Do not use GPLv2+, but GPLv3+
+# Done upstream
+Patch0: license-use-consistently-GPLv3.patch
+
+# Do not have libgd.so
+# Done upstream
+Patch1: static-libgd.patch

     => Just a random note, for the future: the guidelines normally recommend a link to
        the upstream bug tracker or mailing-list where the patches have been submitted
        or to the commits in the git web interface.
     => But given that you're upstream, and that a new release (3.7.92) will be out soon,
        I certainly wouldn't block the review on this. Just for your information. :)

 BuildRequires:	intltool
 BuildRequires:	itstool
+BuildRequires:	gettext
 BuildRequires:	glib2-devel >= 2.28.0
 BuildRequires:	gtk3-devel >= 3.5.19
 BuildRequires:	tracker-devel => 0.15.2
@@ -17,25 +28,40 @@
 BuildRequires:	libzeitgeist-devel >= 0.3.18
 BuildRequires:	webkitgtk3-devel >= 1.11.91
 BuildRequires:	libuuid-devel
+BuildRequires:	yelp-tools
+
+# libgd is not meant to be installed as a system-wide shared library.
+# It is just a way for GNOME applications to share widgets and other common
+# code on an ad-hoc basis.
+Provides: bundled(libgd)

     => Good marking of bundled library.
 
 %description
 Simple note editor which emphasis on visuals : quickly write
 notes, quickly find it back.
 
+
 %prep
 %setup -q
+%patch0 -p1
+%patch1 -p1
+
+autoreconf -i -f
 
 %build
 %configure \
 	--disable-static
-make
+make %{?_smp_mflags}
+
     => Fixes parallel building.
 
 %install
-rm -rf %{buildroot}

     => Good riddance. :)

-make DESTDIR=%{buildroot} install
+make install DESTDIR=%{buildroot} INSTALL="/usr/bin/install -p"

     => Fixes preserving of timestamps.

-%clean
-rm -rf %{buildroot}

     => Good riddance. :)

+# Validates the .desktop
+desktop-file-validate %{buildroot}%{_datadir}/applications/%{name}.desktop

     => Good validation of the desktop file.
     => However, the package doesn't build any more, because you forgot to add
            BuildRequires: desktop-file-utils

+# Creates the file for all locales
+%find_lang %{name} --with-gnome

     => Good handling of locale files.

 %post
 /usr/bin/update-desktop-database &> /dev/null || :

     => You're missing the "/bin/touch --no-create ..." line in %post

@@ -44,48 +70,59 @@
 /usr/bin/update-desktop-database &> /dev/null || :
 if [ $1 -eq 0 ] ; then
     /usr/bin/glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || :
+    /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null
+    /usr/bin/gtk-update-icon-cache -f %{_datadir}/icons/hicolor &>/dev/null || :
 fi

     => Good handling of installed icons.
 
 %posttrans
-    /usr/bin/glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || :
+/usr/bin/glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || :
+/usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :

     => Good handling of installed icons.

 %files
 %doc NEWS AUTHORS COPYING ChangeLog NEWS README
+%files -f %{name}.lang

     => Good handling of the locale files.

 %{_bindir}/%{name}
 %{_datadir}/applications/%{name}.desktop
 %{_datadir}/glib-2.0/schemas/org.gnome.bijiben.gschema.xml
+# co-own these directories
+%dir %{_datadir}/gnome-shell
+%dir %{_datadir}/gnome-shell/search-providers

     => Fixes directory ownership.

 %{_datadir}/gnome-shell/search-providers/bijiben-search-provider.ini
-%{_datadir}/help/C/%{name}

     => Good handling of the locale files.

 %{_datadir}/icons/hicolor/*/apps/%{name}.png
 %{_datadir}/icons/hicolor/*/apps/%{name}.svg
-%lang(ca) %{_datadir}/locale/ca/LC_MESSAGES/%{name}.mo
-%lang(cs) %{_datadir}/locale/cs/LC_MESSAGES/%{name}.mo
-%lang(es) %{_datadir}/locale/es/LC_MESSAGES/%{name}.mo
-%lang(fr) %{_datadir}/locale/fr/LC_MESSAGES/%{name}.mo
-%lang(gl) %{_datadir}/locale/gl/LC_MESSAGES/%{name}.mo
-%lang(it) %{_datadir}/locale/it/LC_MESSAGES/%{name}.mo
-%lang(pl) %{_datadir}/locale/pl/LC_MESSAGES/%{name}.mo
-%lang(pt) %{_datadir}/locale/pt_BR/LC_MESSAGES/%{name}.mo
-%lang(sl) %{_datadir}/locale/sl/LC_MESSAGES/%{name}.mo
-%lang(sr) %{_datadir}/locale/sr/LC_MESSAGES/%{name}.mo
-%lang(sr) %{_datadir}/locale/sr@latin/LC_MESSAGES/%{name}.mo
-%lang(zh) %{_datadir}/locale/zh_CN/LC_MESSAGES/%{name}.mo

     => Good handling of the locale files.

 %{_datadir}/%{name}
-%{_libdir}/%{name}/libgd.so

     => Great, you don't install it anymore, which fixes the unowned directory, as
        well as the bogus Requires/Provides.

 %{_libexecdir}/%{name}-shell-search-provider
 %{_datadir}/dbus-1/services/org.gnome.Bijiben.SearchProvider.service
 %exclude %{_libdir}/%{name}/libgd.la
 
 %changelog
+* Wed Mar 13 2013 Pierre-Yves Luyten <py@luyten.fr> - 3.7.91-2
+- Patch to get rid of GPLv2+
+- Patch to use static link to libgd
+- Use find_lang
+- Remove define for url_ver
+- desktop-file-validate
+- update timestamp for icons
+- BuildRequire gettext
+- BuildRequire yelp-tools
+- Provides bundled libgd
+- Don't "clean"
+
 * Sun Mar 10 2013 Pierre-Yves Luyten <py@luyten.fr> - 3.7.91-1
 - Fix BuildRequires
 - Update desktop database
 - Add it
+
 * Sun Feb 17 2013 Pierre-Yves Luyten <py@luyten.fr> - 3.7.90-1
 - Bump release
+
 * Sat Feb 02 2013 Pierre-Yves Luyten <py@luyten.fr> - 3.7.5-1
 - Add cs. 
+
 * Mon Jan 14 2013 Pierre-Yves Luyten <py@luyten.fr> - 3.7.4-1
 - Add ca pt zh. Remove upstreamed patch.
+
 * Mon Dec 17 2012 Pierre-Yves Luyten <py@luyten.fr> - 3.7.3-1
 - Initial package
Comment 11 Mathieu Bridon 2013-03-15 04:45:05 EDT
Ok, the package looks very good now! :)

If I could, I would approve your package, provided that the legal team confirms I didn't make any mistake with that License tag.

I wrote to the legal team to have a quick confirmation:
    http://lists.fedoraproject.org/pipermail/legal/2013-March/002109.html


Remaining issues:
================

[!]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
     => The license tag appears correct to me, but it is more complex than what I've
        been used to, so I'd appreciate a confirmation from the legal team.


Details:
========

--- bijiben.spec.bak	2013-03-15 12:57:11.441577743 +0800
+++ bijiben.spec	2013-03-15 15:54:42.000000000 +0800
@@ -1,36 +1,37 @@
 Name:		bijiben
 Version:	3.7.91
-Release:	2%{?dist}
+Release:	3%{?dist}
 Summary:	Simple Note Viewer
 
-# Bijiben is GPLv3+ appart a few files "LGPLv2 or LGPLv3"
+# Bijiben is GPLv3+ apart a few files "LGPLv2 or LGPLv3"
 # And ligd is LGPLv2+
 License:	GPLv3+ and LGPLv3 and LGPLv2+
 Url:		http://www.gnome.org
 Source0:	http://ftp.gnome.org/pub/GNOME/sources/%{name}/3.7/%{name}-%{version}.tar.xz
 
 # Do not use GPLv2+, but GPLv3+
-# Done upstream
+# Done upstream : commit 45e77b6fbdd46873ef9de7e1b8caf41a22a2c6d0
 Patch0: license-use-consistently-GPLv3.patch
 
 # Do not have libgd.so
-# Done upstream
+# Done upstream : commit 9f6bd4e1101a43e9a25133e74500b462bc361f5e
 Patch1: static-libgd.patch
 
-BuildRequires:	intltool
+BuildRequires:	clutter-gtk-devel >=  1.4.2
+BuildRequires:	desktop-file-utils

     => Fixes validation of the desktop file

+BuildRequires:	intltool
 BuildRequires:	itstool
 BuildRequires:	gettext
 BuildRequires:	glib2-devel >= 2.28.0
 BuildRequires:	gtk3-devel >= 3.5.19
-BuildRequires:	tracker-devel => 0.15.2
-BuildRequires:	clutter-gtk-devel >=  1.4.2
+BuildRequires:	libuuid-devel
 BuildRequires:	libxml2-devel
 BuildRequires:	libzeitgeist-devel >= 0.3.18
+BuildRequires:	tracker-devel => 0.15.2
 BuildRequires:	webkitgtk3-devel >= 1.11.91
-BuildRequires:	libuuid-devel
 BuildRequires:	yelp-tools
 
+
 # libgd is not meant to be installed as a system-wide shared library.
 # It is just a way for GNOME applications to share widgets and other common
 # code on an ad-hoc basis.
@@ -66,6 +67,7 @@
 
 %post
 /usr/bin/update-desktop-database &> /dev/null || :
+/bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :

     => Fixes handling of installed icons.

 %postun
 /usr/bin/update-desktop-database &> /dev/null || :
@@ -99,6 +101,10 @@
 %exclude %{_libdir}/%{name}/libgd.la
 
 %changelog
+* Fri Mar 15 2013 Pierre-Yves Luyten <py@luyten.fr> - 3.7.91-3
+- Fix icon update (timestamp) in post
+- BuildRequire desktop-file-utils
+
 * Wed Mar 13 2013 Pierre-Yves Luyten <py@luyten.fr> - 3.7.91-2
 - Patch to get rid of GPLv2+
 - Patch to use static link to libgd
Comment 12 Pierre-Yves Luyten 2013-03-18 00:29:59 EDT
New minor release, mostly removing patches

SPEC: http://py.luyten.fr/Publique/gnome/bjb/3792-1/bijiben.spec
SRPM: http://py.luyten.fr/Publique/gnome/bjb/3792-1/bijiben-3.7.92-1.fc19.src.rpm



$ diff 3791-3 37921 
2,3c2,3
< Version:	3.7.91
< Release:	3%{?dist}
---
> Version:	3.7.92
> Release:	1%{?dist}
12,19d11
< # Do not use GPLv2+, but GPLv3+
< # Done upstream : commit 45e77b6fbdd46873ef9de7e1b8caf41a22a2c6d0
< Patch0: license-use-consistently-GPLv3.patch
< 
< # Do not have libgd.so
< # Done upstream : commit 9f6bd4e1101a43e9a25133e74500b462bc361f5e
< Patch1: static-libgd.patch
< 
48,49d39
< %patch0 -p1
< %patch1 -p1
103a94,96
> * Mon Mar 18 2013 Pierre-Yves Luyten <py@luyten.fr> - 3.7.92-1
> - New release
>
Comment 13 Matthias Clasen 2013-03-20 07:16:24 EDT
I can sponsor you
Comment 14 Tom "spot" Callaway 2013-03-20 13:29:29 EDT
License is correct. Lifting FE-Legal.
Comment 15 Mathieu Bridon 2013-03-20 21:59:33 EDT
(In reply to comment #14)
> License is correct. Lifting FE-Legal.

Thanks Tom!

(In reply to comment #13)
> I can sponsor you

Like I said in comment 11, the package was in a state where I'd approve it if Pierre-Yves didn't need a sponsor (or if I was a sponsor myself).

So there's no need for me any more here, he's all yours Matthias. :)
Comment 16 Pierre-Yves Luyten 2013-03-26 21:23:30 EDT
Not much change below, yes sponsoring would be nice now the version is stable.

SPEC: http://py.luyten.fr/Publique/gnome/bjb/380-1/bijiben.spec
SRPM: http://py.luyten.fr/Publique/gnome/bjb/380-1/bijiben-3.8.0-1.fc20.src.rpm
Comment 17 Pierre-Yves Luyten 2013-04-18 05:18:48 EDT
The manual switched to creative-commons and received small fixes

SPEC: http://py.luyten.fr/Publique/gnome/bjb/381-1/bijiben.spec
SRPM: http://py.luyten.fr/Publique/gnome/bjb/381-1/bijiben-3.8.1-1.fc19.src.rpm
Comment 18 Ankur Sinha (FranciscoD) 2013-04-25 00:38:52 EDT
Hi Pierre,

The package's already been reviewed and looks good. I've just built myself an rpm and am using it on my F19 system. Works well too. Looking forward to having it in Fedora, as soon as Matthias sponsors you and approves the review :)

Thanks,
Warm regards,
Ankur
Comment 19 Pierre-Yves Luyten 2013-05-01 09:57:44 EDT
I built 3.9.1 against rawhide.

SPEC : http://py.luyten.fr/Publique/gnome/bjb/391-1/bijiben.spec
SRPM : http://py.luyten.fr/Publique/gnome/bjb/391-1/bijiben-3.9.1-1.fc20.src.rpm

For F19, there is still 3.8.2 to come.
Comment 20 Kalev Lember 2013-05-06 16:24:51 EDT
Hi Pierre,

I have talked to mclasen and I'll handle your sponsoring to the Fedora packager group. Thanks for all the work both here and upstream!

From a quick look at the packaging, it looks pretty much ready for Fedora. I'll go through the review checklist later, but in the mean time if you have a few spare moments, there's something you could do:

All Fedora packagers are also reviewers. When someone posts a package for review, it's often that they swap reviews with someone else, just to keep the new package review queue from overflowing.

Would be great if you could do an unofficial review of something just to get a taste of what it is. You aren't sponsored yet, so you won't be able to mark the review as passed (I'll do that for you), but you could go through the review checklist and post your findings.

https://fedoraproject.org/wiki/Features/Gnome3.8#Scope has a list of new GNOME 3.8 packages. A few of the games there are still missing a review; if you want to do one of these, they should be nice and simple.

You can catch me on irc as kalev or send me an email (kalevlember@gmail.com) if you have any questions.

Thanks again and welcome to Fedora!

References:
https://fedoraproject.org/wiki/Package_Review_Process
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
https://fedoraproject.org/wiki/Packaging:Guidelines
Comment 21 Kalev Lember 2013-05-08 10:20:58 EDT
Alright, I see you've done an (unofficial) review for quadrapassel (bug 920875) and it looks good to me. I have now sponsored you to the packager group, use your new powers visely! Feel free to go ahead and approve quadrapassel officially. (Your new permissions might take up to an hour to sync before you can do that, though.)
Comment 22 Kalev Lember 2013-05-08 10:26:52 EDT
A few comments / questions about this package:


1) The name and summary in the spec file should match up with the name and summary in the review ticket (and also with the name and summary in the SCM request that you are going to file soon). Right now the ticket here has:

Review Request: Bijiben - Note taking app

... and the spec file:

Name:           bijiben
Summary:        Simple Note Viewer

Can you update the ticket here with the lowercase 'bijiben' name and sync up the descriptions? The scripts for creating new git repos check these.


2)
> autoreconf -i -f

The autoreconf call seems unneeded now that you are no longer patching the build system.


3)
> %exclude %{_libdir}/%{name}/libgd.la

Shouldn't be needed any more now that the package is no longer installing the libgd shared library.


4)
> %files
> %doc NEWS AUTHORS COPYING ChangeLog NEWS README
> %files -f %{name}.lang

Two issues here: you are repeating the %files section and the NEWS file is listed twice. It should read:

%files -f %{name}.lang
%doc AUTHORS COPYING ChangeLog NEWS README


5)
/usr/share/bijiben/icons
/usr/share/bijiben/icons/hicolor
/usr/share/bijiben/icons/hicolor/16x16
/usr/share/bijiben/icons/hicolor/16x16/actions
/usr/share/bijiben/icons/hicolor/16x16/actions/note.png
/usr/share/bijiben/icons/hicolor/24x24
/usr/share/bijiben/icons/hicolor/24x24/actions
/usr/share/bijiben/icons/hicolor/24x24/actions/note.png
/usr/share/bijiben/icons/hicolor/48x48
/usr/share/bijiben/icons/hicolor/48x48/actions
/usr/share/bijiben/icons/hicolor/48x48/actions/note.png
/usr/share/bijiben/icons/hicolor/scalable
/usr/share/bijiben/icons/hicolor/scalable/actions
/usr/share/bijiben/icons/hicolor/scalable/actions/link.svg
/usr/share/bijiben/icons/hicolor/scalable/actions/note.svg


Are these supposed to get installed in /usr/share/bijiben ? The directory layout looks like they should have gone to /usr/share/icons/hicolor instead.
Comment 23 Mathieu Bridon 2013-05-08 10:56:39 EDT
(In reply to comment #22)
> 5)
> /usr/share/bijiben/icons
> /usr/share/bijiben/icons/hicolor
> /usr/share/bijiben/icons/hicolor/16x16
> /usr/share/bijiben/icons/hicolor/16x16/actions
> /usr/share/bijiben/icons/hicolor/16x16/actions/note.png
> /usr/share/bijiben/icons/hicolor/24x24
> /usr/share/bijiben/icons/hicolor/24x24/actions
> /usr/share/bijiben/icons/hicolor/24x24/actions/note.png
> /usr/share/bijiben/icons/hicolor/48x48
> /usr/share/bijiben/icons/hicolor/48x48/actions
> /usr/share/bijiben/icons/hicolor/48x48/actions/note.png
> /usr/share/bijiben/icons/hicolor/scalable
> /usr/share/bijiben/icons/hicolor/scalable/actions
> /usr/share/bijiben/icons/hicolor/scalable/actions/link.svg
> /usr/share/bijiben/icons/hicolor/scalable/actions/note.svg
> 
> 
> Are these supposed to get installed in /usr/share/bijiben ? The directory
> layout looks like they should have gone to /usr/share/icons/hicolor instead.

I don't think so, or at least, they should probably be renamed then, to avoid conflicts with other packages. (these names are too generic for a shared location)

But is there something in the guidelines that forbids installing icons here? That seems perfectly fine to me.

(I'm asking because I thought it was fine when I did the initial review, and I'd be happy to learn if I was wrong :)
Comment 24 Kalev Lember 2013-05-08 11:22:20 EDT
No, nothing wrong with it, it's perfectly fine :) Just wanted to doublecheck that it's not a typo, easy to accidentally use pkgdatadir instead of datadir.
Comment 25 Pierre-Yves Luyten 2013-05-08 16:13:40 EDT
Thanks for look up & sponsoring, I will gather some time to contribute on other reviews. I also need to play with mock.


Regarding 1) I just updated the ticket.

Regarding 5), the icons : indeed the path is "/usr/share/bijiben/icons/hicolor" - as Mathieu writes, it avoids conflicts.

For 2), 3), 4) - I'll submit update soon
Comment 26 Pierre-Yves Luyten 2013-05-08 19:05:51 EDT
(In reply to comment #22)

> 2)
> > autoreconf -i -f
> 
> The autoreconf call seems unneeded now that you are no longer patching the
> build system.
> 


ah yes, done.


> 3)
> > %exclude %{_libdir}/%{name}/libgd.la
> 
> Shouldn't be needed any more now that the package is no longer installing
> the libgd shared library.

Yes, no lib is installed, removed.

> 
> 4)
> > %files
> > %doc NEWS AUTHORS COPYING ChangeLog NEWS README
> > %files -f %{name}.lang
> 
> Two issues here: you are repeating the %files section and the NEWS file is
> listed twice.

I can't believe i missed it, thanks.

SPEC: http://py.luyten.fr/Publique/gnome/bjb/381-2/bijiben.spec
SRPM: http://py.luyten.fr/Publique/gnome/bjb/381-2/bijiben-3.8.1-2.fc19.src.rpm
Comment 27 Kalev Lember 2013-05-08 19:15:34 EDT
Looks good.

APPROVED
Comment 28 Pierre-Yves Luyten 2013-05-08 19:43:43 EDT
New Package SCM Request
=======================
Package Name: bijiben
Short Description: Simple Note Viewer
Owners: pyluyten
Branches: f19
InitialCC:
Comment 29 Mathieu Bridon 2013-05-08 22:49:12 EDT
Pierre-Yves, you had forgotten to set the fedrora-cvs flag to "?", as such the SCM admins would never have found about the request.

I just did it for you, because I'm eager to have Bijiben in Fedora, but remember it for your next review. :)

(in case you don't see it, it's the bottom-right field in the top form of each bug reports)
Comment 30 Jon Ciesla 2013-05-09 07:04:09 EDT
Git done (by process-git-requests).
Comment 31 Pierre-Yves Luyten 2013-05-09 07:53:19 EDT
(In reply to comment #29)
> Pierre-Yves, you had forgotten to set the fedrora-cvs flag to "?"

> I just did it for you


Yes thanks Mathieu, i could not amend the flag "yesterday" (i mean 2013-05-08 19:43:43 EDT). As of now i have access to the flag.
Comment 32 Fedora Update System 2013-05-09 19:57:21 EDT
bijiben-3.8.1-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/bijiben-3.8.1-2.fc19