Bug 827664 - Review Request: zukini - Themes for GTK+2, GTK+3, Metacity and GNOME Shell
Review Request: zukini - Themes for GTK+2, GTK+3, Metacity and GNOME Shell
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-02 02:49 EDT by Mattia M.
Modified: 2012-06-19 20:32 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-10 15:52:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
michel: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mattia M. 2012-06-02 02:49:35 EDT
Spec URL: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.02/zukini.spec

SRPM URL: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.02/zukini-20120526-1.fc17.src.rpm

Description: The Zukini themes for GTK+2, GTK+3, Metacity and GNOME Shell, created by lassekongo83.

Fedora Account System Username: odysseus
Comment 2 Michel Alexandre Salim 2012-06-02 11:23:45 EDT
You're packaging another of my favorite themes :)

Will review tomorrow -- I have a simple package for you to review as well, if you don't mind (will post the review request when I get home)
Comment 3 Michel Alexandre Salim 2012-06-02 12:39:14 EDT
(In reply to comment #2)
> Will review tomorrow -- I have a simple package for you to review as well,
> if you don't mind (will post the review request when I get home)

Here's the review in question, it's for a simple Python library. Mostly a straightforward adaptation of the Python packaging guidelines:

  http://fedoraproject.org/wiki/Packaging:Python

but I had to jury-rig a %check section since upstream didn't provide any; it basically find every possible loadable module and try to load each of them - it'd fail if the dependencies are not complete.
Comment 4 Michel Alexandre Salim 2012-06-02 13:02:13 EDT
OK, here's the review. I think basically everything is in order -- *except* that upstream has again issued an update (on Saturday June 2nd), and DeviantArt reuses the same download URL, so the file size, rpmlint warning, and "not packaging the latest version" are all due to the same cause.

Bump the spec's version field and include the latest zip archive, and I'll approve this.



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

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[-]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[-]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[-]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: 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 is included in %doc.
[!]: MUST License field in the package spec file matches the actual license.
[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

rpmlint zukini-gtk3-theme-20120526-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint zukini-gtk2-theme-20120526-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint zukini-20120526-1.fc17.src.rpm

zukini.src: W: file-size-mismatch zukini_by_lassekongo83-d4ic1u2.zip = 196528, http://www.deviantart.com/download/272660042/zukini_by_lassekongo83-d4ic1u2.zip = 196521
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gnome-shell-theme-zukini-20120526-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint zukini-metacity-theme-20120526-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/tmp/827664/zukini_by_lassekongo83-d4ic1u2.zip :
  MD5SUM this package     : 734247c5185d47c24bb6818c906a15b7
  MD5SUM upstream package : 3f36ea80f97fc383c223a999ef104892

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[-]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: 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.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[!]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[!]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[!]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Rpmlint output is silent.

rpmlint zukini-gtk3-theme-20120526-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint zukini-gtk2-theme-20120526-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint zukini-20120526-1.fc17.src.rpm

zukini.src: W: file-size-mismatch zukini_by_lassekongo83-d4ic1u2.zip = 196528, http://www.deviantart.com/download/272660042/zukini_by_lassekongo83-d4ic1u2.zip = 196521
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gnome-shell-theme-zukini-20120526-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint zukini-metacity-theme-20120526-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/tmp/827664/zukini_by_lassekongo83-d4ic1u2.zip :
  MD5SUM this package     : 734247c5185d47c24bb6818c906a15b7
  MD5SUM upstream package : 3f36ea80f97fc383c223a999ef104892

See: http://fedoraproject.org/wiki/Packaging/SourceURL


Generated by fedora-review 0.1.3
External plugins:
Comment 5 Michel Alexandre Salim 2012-06-02 14:49:28 EDT
(In reply to comment #4)
> OK, here's the review. I think basically everything is in order -- *except*
> that upstream has again issued an update (on Saturday June 2nd), and
> DeviantArt reuses the same download URL, so the file size, rpmlint warning,
> and "not packaging the latest version" are all due to the same cause.
> 
> Bump the spec's version field and include the latest zip archive, and I'll
> approve this.
> 
Ah, and one more thing -- shouldn't the license be GPLv3, without the +? I couldn't find any reference to the "or later" clause in the documentation and other text files; if you do see any, please let me know.
Comment 6 Mattia M. 2012-06-03 06:23:22 EDT
(In reply to comment #4)
> OK, here's the review. I think basically everything is in order -- *except*
> that upstream has again issued an update (on Saturday June 2nd), and
> DeviantArt reuses the same download URL, so the file size, rpmlint warning,
> and "not packaging the latest version" are all due to the same cause.
> 
> Bump the spec's version field and include the latest zip archive, and I'll
> approve this.

OK, I've updated everything to the latest release.

(In reply to comment #5)
> Ah, and one more thing -- shouldn't the license be GPLv3, without the +? I
> couldn't find any reference to the "or later" clause in the documentation
> and other text files; if you do see any, please let me know.

You're right: I've also changed the "License" entry to "GPLv3" in the Spec file.
Thank you! 

Spec URL: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.03/zukini.spec

SRPM URL: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.03/zukini-20120602-1.fc17.src.rpm

( RPM packages for Fedora 17: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.03/RPMs/ )

P.S.: I'll try to review your package in the next days, but I can't promise you anything.
Comment 7 Michel Alexandre Salim 2012-06-04 00:52:07 EDT
All previously raised issues addressed -- just one more thing (related to the comment Artyom made in the Zukitwo review): there are references to Ubuntu-specific things in both Zukini and Zukitwo that needs to be fixed:


➜  themes  grep -r "font-family: Ubuntu" Zuki*
Zukini/gnome-shell/gnome-shell.css:	font-family: Ubuntu, Droid Sans, sans-serif;
Zukitwo-Shell/gnome-shell/gnome-shell.css:	font-family: Ubuntu, Droid Sans, sans-serif;
➜  themes  grep -r unity Zuki*                
Zukini/gtk-3.0/gtk.css:@import url("apps/unity.css");
Zukitwo/gtk-3.0/gtk.css:@import url("apps/unity.css");
Zukitwo-Dark/gtk-3.0/gtk.css:@import url("apps/unity.css");

The font reference is not so bad as it'd fall back to the next font in line (either Droid Sans -- available but not installed by default, or sans-serif) but the unity reference causes a warning to be outputted to the console every time a GTK3 app is started:

(evince:6289): Gtk-WARNING **: Theme parsing error: gtk.css:60:30: Failed to import: Error opening file: No such file or directory

So it's probably best to carry a patch that when applied would remove that line (in both themes), and apply that patch during the %setup phase.
Comment 8 Mattia M. 2012-06-04 02:50:19 EDT
(In reply to comment #7)
> All previously raised issues addressed -- just one more thing (related to
> the comment Artyom made in the Zukitwo review): there are references to
> Ubuntu-specific things in both Zukini and Zukitwo that needs to be fixed:
> 
> 
> ➜  themes  grep -r "font-family: Ubuntu" Zuki*
> Zukini/gnome-shell/gnome-shell.css:	font-family: Ubuntu, Droid Sans,
> sans-serif;
> Zukitwo-Shell/gnome-shell/gnome-shell.css:	font-family: Ubuntu, Droid Sans,
> sans-serif;
> ➜  themes  grep -r unity Zuki*                
> Zukini/gtk-3.0/gtk.css:@import url("apps/unity.css");
> Zukitwo/gtk-3.0/gtk.css:@import url("apps/unity.css");
> Zukitwo-Dark/gtk-3.0/gtk.css:@import url("apps/unity.css");
> 
> The font reference is not so bad as it'd fall back to the next font in line
> (either Droid Sans -- available but not installed by default, or sans-serif)
> but the unity reference causes a warning to be outputted to the console
> every time a GTK3 app is started:
> 
> (evince:6289): Gtk-WARNING **: Theme parsing error: gtk.css:60:30: Failed to
> import: Error opening file: No such file or directory
> 
> So it's probably best to carry a patch that when applied would remove that
> line (in both themes), and apply that patch during the %setup phase.

Thanks for your suggestions.
The best solution for me would be removing "Ubuntu" from "font-family" (through a patch, as you suggest) and adding "google-droid-fonts" as a requirement.
Do you agree?
Comment 9 Mattia M. 2012-06-05 06:42:46 EDT
I wrote to lassekongo83, the author of both Zukini and Zukitwo, about the font issue:

"[...] Both the GNOME Shell themes use "Ubuntu" font as default.
Since this is not installed or available in the official repositories, which font do you suggest as an alternative? The theme automatically falls back to "Droid Sans", which is quite good-looking, but, in my opinion, also "Cantarell" (the default font of GNOME 3) suits well.
What do you think? [...]"

He answered:

"Droid sans is the best in my opinion after Ubuntu. Cantarell somehow looks too "amateurish" (or a little bit cartoonish) for my taste. Another alternative would be Liberation Sans. [...]"

So, now I think I should change font-family to the following:

"font-family: Droid Sans, Liberation Sans, sans-serif;"

and add "google-droid-sans-fonts" and "liberation-sans-fonts" as requirements.
Comment 10 Michel Alexandre Salim 2012-06-05 12:05:58 EDT
Sounds good, yes (Liberation Sans is not Fedora's default sans font - though it's LibreOffice's - so adding it to the list is indeed necessary)
Comment 11 Michel Alexandre Salim 2012-06-05 12:53:06 EDT
per upstream's communication, I guess Liberation Sans will be in the list of font suggestions in the next update, which is good.

Do we need having liberation-sans-fonts as a requirement though? We pull in google-droid-sans-fonts, users get the first choice (of the patched list) or the second choice of the original list -- if they want to change the font and look at the CSS file, surely they'd be able to install Liberation Sans by hand if they don't already have it.

So I think patching the font-family line and depending on google-droid-sans-fonts is enough.
Comment 12 Mattia M. 2012-06-05 16:12:12 EDT
This is the Zukitwo package, modified as suggested: http://koji.fedoraproject.org/koji/buildinfo?buildID=322028

As soon as I have enough time, I'm going to modify the Zukini package in the same way.
Comment 13 Mattia M. 2012-06-07 00:15:55 EDT
(In reply to comment #12)
> This is the Zukitwo package, modified as suggested:
> http://koji.fedoraproject.org/koji/buildinfo?buildID=322028
> 
> As soon as I have enough time, I'm going to modify the Zukini package in the
> same way.

And here is the updated Zukini package:

Spec URL: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.07/zukini.spec

SRPM URL: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.07/zukini-20120602-2.fc17.src.rpm

( RPM packages for Fedora 17: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.07/RPMs/ )
Comment 14 Michel Alexandre Salim 2012-06-07 03:30:09 EDT
Aha, thanks! Could you also patch gtk-3.0/gtk.css in both themes? You delete apps/unity.css but they're still referenced in gtk.css -- so launching any GTK3 app results in a noisy error on the console.

Will approve once that is done -- sorry this is taking a while :/
Comment 15 Mattia M. 2012-06-07 03:49:09 EDT
(In reply to comment #14)
> Aha, thanks! Could you also patch gtk-3.0/gtk.css in both themes? You delete
> apps/unity.css but they're still referenced in gtk.css -- so launching any
> GTK3 app results in a noisy error on the console.

Damn! I didn't notice that! ;-)

> Will approve once that is done -- sorry this is taking a while :/

No problem: I don't mind waiting, if this means to improve quality of the package.
Thanks again!
Comment 16 Mattia M. 2012-06-09 04:26:58 EDT
Hoping there are no other issues, here's the latest version of Zukini:

Spec URL: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.09/zukini.spec

SRPM URL: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.09/zukini-20120607-1.fc17.src.rpm

( RPM packages for Fedora 17: http://odysseus.fedorapeople.org/packages/Zukini/2012.06.09/RPMs/ )
Comment 17 Michel Alexandre Salim 2012-06-09 08:43:55 EDT
Everything looks fine now, thanks!

APPROVED
Comment 18 Mattia M. 2012-06-09 15:47:09 EDT
(In reply to comment #17)
> Everything looks fine now, thanks!
> 
> APPROVED

Thank you!
Comment 19 Mattia M. 2012-06-09 15:50:00 EDT
New Package SCM Request
=======================
Package Name: zukini
Short Description: The Zukini themes for GTK+2, GTK+3, Metacity and GNOME Shell, created by lassekongo83.
Owners: odysseus
Branches: f17
InitialCC:
Comment 20 Mattia M. 2012-06-10 03:41:01 EDT
(In reply to comment #17)
> Everything looks fine now, thanks!
> 
> APPROVED

Thanks for your assistance and helpfulness.
You forgot to include a link to the package you were asking me to review, so I chose the following one and made a "review draft" of it: https://bugzilla.redhat.com/show_bug.cgi?id=827809
Comment 21 Gwyn Ciesla 2012-06-10 10:18:43 EDT
Git done (by process-git-requests).
Comment 22 Mattia M. 2012-06-10 15:52:33 EDT
I'm closing this review request.
Thank you all.
Comment 23 Fedora Update System 2012-06-10 15:57:28 EDT
zukini-20120607-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/zukini-20120607-1.fc17
Comment 24 Fedora Update System 2012-06-19 20:32:07 EDT
zukini-20120607-1.fc17 has been pushed to the Fedora 17 stable repository.

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