Bug 769096

Hello Yader,

I would like to provide an informal review for this package.


rpmlint shows the following errors and warnings for the SRPM:

rpmlint SRPMS/gnome-shell-extension-windowoverlay-icons-3.0-1.fc16.src.rpm 
gnome-shell-extension-windowoverlay-icons.src: W: summary-ended-with-dot C Easily discover which application to select by viewing the app icons in the windows overview.
gnome-shell-extension-windowoverlay-icons.src: E: summary-too-long C Easily discover which application to select by viewing the app icons in the windows overview.
gnome-shell-extension-windowoverlay-icons.src: E: description-line-too-long C This extension allow to view the icons over the application in the windows overview.
gnome-shell-extension-windowoverlay-icons.src: E: description-line-too-long C Useful to avoid confusion with the windows when you have a lot of them open on the
1 packages and 0 specfiles checked; 3 errors, 1 warnings.

No errors or warnings for the SPEC file.


Using wget to download the sources from URL listed as Source0 doesn't return a tar file with proper name. I think using a comment to describe how to generate the tarball and using that tarball as Source is better [1]. I propose the following changes:

# wget https://github.com/sustmi/gnome-shell-extensions-sustmi/tarball/%%{git} -O %%{name}-%%{version}-%%{git}.tar.gz
Source0:                %%{name}-%%{version}-%%{git}.tar.gz

With the above changes it builds on mock with target fedora-15. A warning is added because no URL is being used in Source0.


1. https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control

I fixed the errors and warnings. About the Source0 I understand what you are saying, but if you use spectool -g pkgspec.spec you can download the source and avoid the warning in mock, just saying. I can change it if you consider is the best practice.

Yader, Thank you for the quick reply.

First I want to be clear about the kind of review I'm doing. I'm still learning the packaging guidelines and I do informal reviews as a way to gain a better understanding of how properly create packages for Fedora. I'm not allowed to approve or reject this package.

At some point someone will do an official review and decide whether the package is ready to be included or not. All we are doing right is trying to improve the package to save some time to the official reviewer.

When you update your spec is expected that you provide the URL for the updated SPEC and SRPM. There should be a different URL for each update and you should increase the release number in the SPEC file and add the corresponding entry to the changelog section.

For the Source0, I didn't consider using spectool. It works. I think the official reviewer should decide which one is the best practice in this case.

If you provide the updated SPEC I will continue with the informal review.

Have you introduced yourself to fedora-devel-list[1][2]?

1. http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Introduce_yourself
2. http://www.redhat.com/mailman/listinfo/fedora-devel-list

Hi Willington, I appreciate your help :)

I updated the spec and srpm.

Spec URL:
http://yaderv.fedorapeople.org/rpm/gnome-shell-windowoverlay-icons/gnome-shell-extension-windowoverlay-icons.spec
SRPM URL:
http://yaderv.fedorapeople.org/rpm/gnome-shell-windowoverlay-icons/gnome-shell-extension-windowoverlay-icons-3.0-2.fc16.src.rpm

Yes, I have already wrote to the list. Regards from Nicaragua.

There is no version 3.0 for this extension. Please use instead the git commit to identify the version of the extension you package.
Many packaged GNOME Shell extensions are in such a case, you can use them as examples to fix your spec file.

I assumed that this was the version number[1], but reading the git logs I don't find any release message.

Are you talking about a pre-release version[2]?

[1] https://github.com/sustmi/gnome-shell-extensions-sustmi/tags
[2] https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

Sorry, I haven't seen the tag ^^.
I suggest you anyway to package instead the latest commit of the extension, which supports both GNOME Shell 3.0 and 3.2.
For the 3.0 version, the metadata.json file indicates that only GNOME Shell 3.0 is supported.

Hi Mohamed, I've packaged it basing on the post-release section in the naming guideline[1]

Spec URL:
http://yaderv.fedorapeople.org/rpm/gnome-shell-windowoverlay-icons/gnome-shell-extension-windowoverlay-icons.spec

SRPM URL:
http://yaderv.fedorapeople.org/rpm/gnome-shell-windowoverlay-icons/gnome-shell-extension-windowoverlay-icons-3.0-3.git72282ce.fc16.src.rpm

Regards :)

[1]https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Post-Release_packages

That looks perfect :).

I've just thinking about something: since the git repository contains in fact two extensions (windowoverlay-icons and historymanager-prefix-search), would you be OK to package both? You'll just have to rename your spec file to  gnome-shell-extensions-sustmi and create a subpackage for each extension.

Yes, I'm not have problem to package both :)

But I have a question about the %install: What is the logic in this step?
Because each sub-package needs to be copied to a different dir with the name of the uuid. I received an error due two %install. 

You can see my test work here: http://yaderv.fedorapeople.org/rpm/gnome-shell-extension-sustmi.spec

Regards

You don't have to use two %install targets per extension (and it doesn't work anyway, as you have seen).  All you have to do is to merge them, as below:

%install
# Install windowoverlay-icons extension
mkdir -p %{buildroot}%{_datadir}/gnome-shell/extensions/%{wiuuid}
install -Dp -m 0644 windowoverlay-icons/{extension.js,metadata.json,stylesheet.css} \
  %{buildroot}%{_datadir}/gnome-shell/extensions/%{wiuuid}/

# Install historymanager-prefix-search extension
mkdir -p %{buildroot}%{_datadir}/gnome-shell/extensions/%{hpsuiid}
install -Dp -m 0644 historymanager-prefix-search/{extension.js,metadata.json,stylesheet.css} \
  %{buildroot}%{_datadir}/gnome-shell/extensions/%{hpsuuid}/


Don't forget also to set a %description for the package; there are only descriptions for each extension. Even if there is no gnome-shell-extension-sustmi package build in fact, a description is needed for the source RPM; for example:

[...]
BuildArch:		noarch
Requires:		gnome-shell

%description
This package provides two GNOME Shell extensions, windowoverlay-icons and historymanager-prefix-search

%package		windowoverlay-icons
[...]


You can drop the buildroot cleaning, as well as the %defattr macro:
   http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
   http://fedoraproject.org/wiki/Packaging:Guidelines#.25clean


Be careful, the macro %{github} used in %prep is no more defined in your latest .spec file.

Hi Mohamed, I've followed your review.

I only have some warnings messages with rpmlint over the srpm

rpmlint gnome-shell-extension-sustmi-3.0-4.git72282ce.fc16.src.rpm
gnome-shell-extension-sustmi.src: W: spelling-error Summary(en_US) windowoverlay -> window overlay, window-overlay, windowpane
gnome-shell-extension-sustmi.src: W: spelling-error Summary(en_US) historymanager -> history manager, history-manager, historiographer
gnome-shell-extension-sustmi.src: W: spelling-error %description -l en_US windowoverlay -> window overlay, window-overlay, windowpane
gnome-shell-extension-sustmi.src: W: spelling-error %description -l en_US historymanager -> history manager, history-manager, historiographer
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

I want to explain that the .spec uses a macro %{github} because the name in the repository has a 's' more.

SRPM: http://yaderv.fedorapeople.org/rpm/gnome-shell-extension-sustmi/gnome-shell-extension-sustmi-3.0-4.git72282ce.fc16.src.rpm

Spec: http://yaderv.fedorapeople.org/rpm/gnome-shell-extension-sustmi/gnome-shell-extension-sustmi.spec

Regards :)

I've fixed an error in the package above

SPEC: http://yaderv.fedorapeople.org/rpm/gnome-shell-extension-sustmi/gnome-shell-extension-sustmi.spec

SRPM: http://yaderv.fedorapeople.org/rpm/gnome-shell-extension-sustmi/gnome-shell-extension-sustmi-3.0-5.git72282ce.fc16.src.rpm

$ rpmlint gnome-shell-extension-sustmi-historymanager-prefix-search-3.0-5.git72282ce.fc16.noarch.rpm
gnome-shell-extension-sustmi-historymanager-prefix-search.noarch: W: spelling-error %description -l en_US eg -> eh, e, g
gnome-shell-extension-sustmi-historymanager-prefix-search.noarch: E: description-line-too-long C Use PageUp and PageDown to move in HistoryManager (eg. RunCommand, Looking Glass)
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

You should fix the description of gnome-shell-extension-sustmi-historymanager-prefix-search. The spellong issues can be ignored.

Don't forget also to modify the description of this review, to comply with the current name of the source package.

Ok, issue solved

SPEC:
http://yaderv.fedorapeople.org/rpm/gnome-shell-extension-sustmi/gnome-shell-extension-sustmi.spec

SRPM:
http://yaderv.fedorapeople.org/rpm/gnome-shell-extension-sustmi/gnome-shell-extension-sustmi-3.0-6.git72282ce.fc16.src.rpm

Description:
This package provides two GNOME Shell extensions, windowoverlay-icons
and historymanager-prefix-search

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.
[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.
[x]: 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 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
[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.
[x]: 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).
[ ]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generates 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 gnome-shell-extension-sustmi-historymanager-prefix-search-3.0-6.git72282ce.fc17.noarch.rpm

gnome-shell-extension-sustmi-historymanager-prefix-search.noarch: W: spelling-error %description -l en_US eg -> eh, e, g
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gnome-shell-extension-sustmi-3.0-6.git72282ce.fc17.src.rpm

gnome-shell-extension-sustmi.src: W: spelling-error Summary(en_US) windowoverlay -> window overlay, window-overlay, windowpane
gnome-shell-extension-sustmi.src: W: spelling-error Summary(en_US) historymanager -> history manager, history-manager, historiographer
gnome-shell-extension-sustmi.src: W: spelling-error %description -l en_US windowoverlay -> window overlay, window-overlay, windowpane
gnome-shell-extension-sustmi.src: W: spelling-error %description -l en_US historymanager -> history manager, history-manager, historiographer
1 packages and 0 specfiles checked; 0 errors, 4 warnings.


rpmlint gnome-shell-extension-sustmi-windowoverlay-icons-3.0-6.git72282ce.fc17.noarch.rpm

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

[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/mohamed/rpmbuild/SPECS/769096/sustmi-gnome-shell-extensions-sustmi-3.0-4-g72282ce.tar.gz :
  MD5SUM this package     : 190f9fc2185a72f514f8a758fe111591
  MD5SUM upstream package : 190f9fc2185a72f514f8a758fe111591

[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.
[x]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: 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.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[x]: 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.
[x]: 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 gnome-shell-extension-sustmi-historymanager-prefix-search-3.0-6.git72282ce.fc17.noarch.rpm

gnome-shell-extension-sustmi-historymanager-prefix-search.noarch: W: spelling-error %description -l en_US eg -> eh, e, g
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gnome-shell-extension-sustmi-3.0-6.git72282ce.fc17.src.rpm

gnome-shell-extension-sustmi.src: W: spelling-error Summary(en_US) windowoverlay -> window overlay, window-overlay, windowpane
gnome-shell-extension-sustmi.src: W: spelling-error Summary(en_US) historymanager -> history manager, history-manager, historiographer
gnome-shell-extension-sustmi.src: W: spelling-error %description -l en_US windowoverlay -> window overlay, window-overlay, windowpane
gnome-shell-extension-sustmi.src: W: spelling-error %description -l en_US historymanager -> history manager, history-manager, historiographer
1 packages and 0 specfiles checked; 0 errors, 4 warnings.


rpmlint gnome-shell-extension-sustmi-windowoverlay-icons-3.0-6.git72282ce.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.
>> This can be ignored, only false-positive spelling issues



Generated by fedora-review 0.1.2
External plugins:

This package is APPROVED!

New Package SCM Request
=======================
Package Name: gnome-shell-extension-sustmi
Short Description: Include two extensions, windowoverlay-icons and historymanager-prefix-search
Owners: yaderv
Branches: f16

Git done (by process-git-requests).

Added f17.

gnome-shell-extension-sustmi-3.0-6.git72282ce.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/gnome-shell-extension-sustmi-3.0-6.git72282ce.fc16

gnome-shell-extension-sustmi-3.0-6.git72282ce.fc16 has been pushed to the Fedora 16 testing repository.

gnome-shell-extension-sustmi-3.0-6.git72282ce.fc16 has been pushed to the Fedora 16 stable repository.