Bug 693370

As these are parsed by scripts, could you please follow the template for package review requests?  I have edited the summary to match what we use.

Thanks.

Sure, sorry. Got a link to the template? I've got more coming..

-c

(In reply to comment #2)
> Sure, sorry. Got a link to the template? I've got more coming..
> 
> -c

Assuming it's this?
https://bugzilla.redhat.com/enter_bug.cgi?product=Fedora&format=fedora-review

-c

Sorry I assigned the bug to me and didn't do anything so far. And I am not going to review the package in it's current state because it should be packaged modular so the theme can be used without a full GNOME desktop. We already heve elemantary-icon-theme packaged.

What I would like to see is:

elementary-gtk-theme - owns %{_datadir}/themes/elementary/gtk-*.0/

elementary-metacity-theme - owns %{_datadir}/themes/elementary/metacity/

xfwm4-theme-elementary or elementary-xfwm4-theme - owns %{_datadir}/themes/elementary/xfwm4/

elementary-gnome-theme - only owns %{_datadir}/themes/elementary/index-theme and requires the icon theme and the metacity theme

You can take a look at the nimbus package, see 
http://pkgs.fedoraproject.org/gitweb/?p=nimbus.git;a=blob;f=nimbus.spec

Hi Chris, do you agree with splitting the package?

Hi Christoph,

Sure, I just haven't had time to work on it yet as I've been away.

Cheers,
-c

Hi Chris, any progress here?

Hi Christoph,

Yes, I've uploaded the new spec files, patch and SRPM to my fedorapeople account.

Spec URL: http://csmart.fedorapeople.org/elementary/elementary.spec
Patch URL: http://csmart.fedorapeople.org/elementary/elementary-fedora.patch
SRPM URL: http://csmart.fedorapeople.org/elementary/elementary-2.1-2.fc15.src.rpm

Thanks,
Chris

Sorry it took so long. The layout of the new package is good, however

- the base name should probably be named elementary-theme or egtk. The letter is the upstream name but I prefer elementary-theme.

- You are not installing any files but only create empty folders

- There are unowned files:
Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/chris/linux/rpmbuild/BUILDROOT/elementary-2.1-2.fc15.x86_64
warning: Installed (but unpackaged) file(s) found:
   /usr/share/themes/elementary/gtk-3.0

Hi Chris,

If I rename the package to elementary-theme, then I can't reference %{name} in the rest of the file, because the resulting packages install to just 'elementary' and not 'elementary-theme' (e.g./usr/share/themes/elementary/metacity-1/). Happy to do it, but just wanted to check first.

I've changed the install command to standard cp -r for directories, rather than going into each directory and listing every file. I guess I should be doing it that way, in the mean time I figured I'd get your feedback on the other things.

The unowned file was because I had a trailing slash on gtk-*.0 under %files, but gtk-3.0 is a softlink. Fixed.

Those are fixed in new SRPM, except for name change at this point. I have also renamed the GNOME and Xfwv4 packages to match the naming convention for GTK and Metacity

Spec URL: http://csmart.fedorapeople.org/elementary/elementary.spec
Patch URL: http://csmart.fedorapeople.org/elementary/elementary-fedora.patch
SRPM URL: http://csmart.fedorapeople.org/elementary/elementary-2.1-3.fc15.src.rpm

Thanks,
-c

REVIEW FOR e99fff15202db7198f75853065b0130a  elementary-2.1-3.fc15.src.rpm


MUST items

FIX - MUST: rpmlint output:

elementary.src: W: name-repeated-in-summary C Elementary
elementary.src: W: no-%build-section
elementary-gnome-theme.noarch: W: spelling-error %description -l en_US metacity -> meta city, meta-city, mendacity
elementary-gnome-theme.noarch: E: description-line-too-long C Desktop theme for GNOME, includes elementary icon theme and elementary metacity (window manager).
elementary-gnome-theme.noarch: W: no-documentation
elementary-gtk-theme.noarch: W: no-documentation
elementary-gtk-theme.noarch: E: backup-file-in-package /usr/share/themes/elementary/gtk-2.0/Apps/panel.rc.~1~
elementary-gtk-theme.noarch: E: backup-file-in-package /usr/share/themes/elementary/gtk-2.0/Apps/postler.rc.~1~
elementary-gtk-theme.noarch: E: backup-file-in-package /usr/share/themes/elementary/gtk-2.0/gtkrc.~1~
elementary-gtk-theme.noarch: E: backup-file-in-package /usr/share/themes/elementary/gtk-2.0/Apps/panel-dark.rc.~1~
elementary-metacity-theme.noarch: W: no-documentation
elementary-xfwm4-theme.noarch: W: no-documentation
5 packages and 0 specfiles checked; 5 errors, 7 warnings.

- Consider adding an empty %build to fix the no-%build-section warning
- Ignore spelling-error warnings
- make sure to insert line breaks after 79 characters to get rid of the  description-line-too-long error
- remove backup files (you could to this in %build, then it has at least a purpose ;) )
- include documentation. The rule of thumb is AUTHORS, CONTRIBUTORS and COPYING and need to be installed for every independent package. You can skip them if they are already installed through a dependency.

OK - MUST: package is named according to the Package Naming Guidelines, but I'd rather prefer the package to be named elementary-theme
OK - MUST: spec file name matches the base package in the format %{name}.spec
FIX - MUST: package meets the Packaging Guidelines. Preserve the timestamps when using install or cp, see https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
OK - MUST: package is licensed with a Fedora approved license and meets the Licensing Guidelines: GPLv2
FIX - MUST: License field in the package spec file does not match the actual license. According to the AUTHORS file this it's GPLv2 (only), not "or any later version".
FIX - MUST: source package includes the text of the license in its own file and that file is included in %doc
OK - MUST: spec file is written in American English
OK - MUST: spec file for the package is legible
OK - MUST: sources match the upstream source, as provided in the spec URL by md5 f685d659c72271944ca2e76d638ca2ed
OK - MUST: package successfully compiles and builds into binary rpms on at least one primary architecture
N/A - 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
OK - MUST: all build dependencies are listed in BuildRequires
N/A - MUST: spec file handles locales properly using %find_lang
N/A - MUST: package (or subpackage) stores shared library files in the dynamic linker's default paths and call ldconfig in %post and %postun
OK - MUST: package does not bundle copies of system libraries
OK - MUST: package is not designed to be relocatable
FIX - MUST: package owns all directories that it creates: You need to own %{_datadir}/themes/elementary or require a package that owns it. See https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
OK - MUST: package does not list a file more than once in the spec file's %files listings
OK - MUST: permissions on files are set properly
FIX - MUST: package does not use macros consistently. You are mixing two different styles, see https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
OK - MUST: package contains code, or permissable content
N/A - MUST: large documentation files must go in a -doc subpackage
OK - MUST: files included as %doc do not affect the runtime of the application
N/A - MUST: header files are in -devel package
N/A - MUST: static libraries are in -static package
N/A - MUST: library files with a suffix are in -devel package
N/A - MUST: -devel package requires the base package using a fully versioned dependency
OK - MUST: package does not contain any .la libtool archives
OK - MUST: package does not include a GUI application, no need for a %{name}.desktop file
OK - MUST: package does not own files or directories already owned by other packages
OK - MUST: all filenames in the package are valid UTF-8


SHOULD items

N/A - SHOULD: source package does not include license text(s) as a separate file from upstream, query upstream to include it
N/A - SHOULD: description and summary sections should contain translations for supported Non-English languages, if available
OK - SHOULD: package builds in mock
OK - SHOULD: package compiles and builds into binary rpms on all supported architectures
OK - SHOULD: package functions as described
N/A - SHOULD: scriptlets are sane
N/A - SHOULD: subpackages other than devel require the base package using a fully versioned dependency
N/A - SHOULD: pkgconfig(.pc) files are in -devel package
OK - SHOULD: package has no file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin
N/A - SHOULD: package contains man pages for binaries/scripts


OTHER items

OK - latest stable version packaged
OK - source URL valid
N/A - compiler flags ok
N/A - debuginfo complete
OK - package has a %clean section, which contains rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ).
N/A - package contains a pkgconfig(.pc) files and has 'Requires: pkgconfig'.


ISSUES

- Fix rpmlint errors/warnings (see above)
- Fix all items marked with FIX
- Strictly speaking elementary-xfwm4-theme and elementary-metacity-theme don't need to require elementary-gtk-theme, but if you remove it from elementary-metacity-theme make sure to add a dependency in elementary-gnome-theme


NOTES

- Consider renaming spec and source package package to elementary-theme. This will make it easier for users when they file bugs because bugzilla uses the srpm name.

Chris, are you still with me?

Yep, but that's a long list and with Christmas holidays wiping out 3 weeks since you replied, I haven't had a chance to look at it yet. I also have a baby due in two weeks so my update to elementary will be delayed, but I'll get there :-)

-c

Hi Chris, I hope your baby got born and is fine. I am sure you have a lot more things to do now than this package. ;)

I therefor close this review. Feel free to reopen it once you have a new package.