Bug 507479 - Review Request: moblin-cursor-theme - Moblin X cursors icon theme
Summary: Review Request: moblin-cursor-theme - Moblin X cursors icon theme
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Christoph Wickert
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FedoraMoblin20
TreeView+ depends on / blocked
 
Reported: 2009-06-22 22:32 UTC by Peter Robinson
Modified: 2009-08-05 10:06 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-08-05 10:06:14 UTC
Type: ---
Embargoed:
christoph.wickert: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Comment 1 Parag AN(पराग) 2009-07-03 09:05:27 UTC
Isn't there any release hosted somewhere for this package?

Comment 2 Fabian Affolter 2009-07-16 09:14:51 UTC
You are mixing '%{buildroot}' and '$RPM_BUILD_ROOT'

Comment 3 Peter Robinson 2009-07-25 09:23:09 UTC
New upstream release

SRPM: http://pbrobinson.fedorapeople.org/moblin-cursor-theme-0.3-1.fc11.src.rpm

Comment 4 Christoph Wickert 2009-08-03 20:57:26 UTC
OK - MUST: $ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/moblin-cursor-theme-0.3-1.fc12.*
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK - MUST: named according to the Package Naming Guidelines
OK - MUST: spec file name matches the base package %{name}
OK - MUST: package meets the Packaging Guidelines
OK - MUST: Fedora approved license and meets the Licensing Guidelines: CC-BY-SA
OK - MUST: license field in spec file matches the actual license
OK - MUST: license file included in %doc
OK - MUST: spec is in American English
OK - MUST: spec is legible
FAIL - MUST: source does not matche upstream source by MD5
  Upstream: 4e88ee79b4aafc08e4dc6defc3dadf7d
  Package: 676eef435c190168bf05b51a24f46772
This is most likely because it's a git snapshot, please use the tarball instead
OK - MUST: successfully compiles and builds into binary rpms on x86_64 (noarch package)
OK - MUST: all build dependencies are listed in BuildRequires (none!)
N/A - MUST: handles locales properly with %find_lang
OK - MUST: not designed to be relocatable
OK - MUST: owns all directories that it creates
OK - MUST: no duplicate files in the %files listing
OK - MUST: permissions on files are set properly, includes %defattr(...)
OK - MUST: package has a %clean section, which contains rm -rf %{buildroot}.
FAIL - MUST: macro usage not consistent: you are using both %{buildroot}/ and $RPM_BUILD_ROOT
OK - MUST: package contains permissable content
OK - MUST: no large docs
OK - MUST: files included as %doc do not affect the runtime
OK - MUST: does not contain any .la libtool archives.
OK - MUST: does not own files or directories already owned by other
packages. In fact it does own %{_datadir}/icons/moblin/ which is already owned by moblin-icon-theme, but since none of the packages requires the other this is ok.
OK - MUST: at the beginning of %install, the package runs rm -rf %{buildroot}.
OK - MUST: all filenames valid UTF-8


SHOULD Items:
OK - 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.
N/A - SHOULD: The description and summary sections in the package spec file
should contain translations for supported Non-English languages, if available.
OK - SHOULD: builds in mock.
OK - SHOULD: package compiles and builds into binary rpms on all supported
architectures.
OK - SHOULD: functions as described.
N/A - SHOULD: scriptlets are sane (no scriptlets used)
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin,
/sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the
file instead of the file itself.


Other items:
OK - latest stable version


Issues:
- Summary: 'Moblin X cursors icon theme' should be just 'Moblin X cursors theme', because cursors are no icons. On the other hand %decription could be more elaborate, e. g.: 'This package contains the Moblin X cursors theme.' or similar.
- don't use a disttag. This is a noarch theme with no dependencies, so it is not necessary to update it during an release upgrade.
- add NEWS to %doc (README is currently not worth adding)
- '%{__mkdir_p} -p' is a duplication. Please don't use macros for simple things like %{__cp} or %{__mkdir_p}. You never know if/how these macros are defined.
- use install rather than cp to make sure permissions are correct and to preserve timestamps
- don't hardcode /usr/share in %install, use %{_datadir}
- Provide the full URL of Source0 and remove the comment on generation of the tarball. This will also fix the wrong MD5, but the tarball needs to be build and we can't use 'make dist' here. Instead, we do a few steps manually:

%build
cd pngs
./make.sh

%install
rm -rf %{buildroot}
mkdir -p %{buildroot}/%{_datadir}/icons/moblin/cursors
for file in xcursors/*; do
  install -pm 0644 $file %{buildroot}/%{_datadir}/icons/moblin/cursors
done

Note that you'll also need 
BuildRequires:  xorg-x11-apps
because it provides xcursorgen

Comment 5 Peter Robinson 2009-08-03 22:29:50 UTC
> FAIL - MUST: source does not matche upstream source by MD5
>   Upstream: 4e88ee79b4aafc08e4dc6defc3dadf7d
>   Package: 676eef435c190168bf05b51a24f46772

No its not. Read the note at the top of the spec file about how to recreate the tarball.

> FAIL - MUST: macro usage not consistent: you are using both %{buildroot}/ and
> $RPM_BUILD_ROOT

Fixed.

> Issues:
> - Summary: 'Moblin X cursors icon theme' should be just 'Moblin X cursors
> theme', because cursors are no icons. On the other hand %decription could be
> more elaborate, e. g.: 'This package contains the Moblin X cursors theme.' or
> similar.
> - don't use a disttag. This is a noarch theme with no dependencies, so it is
> not necessary to update it during an release upgrade.

Fixed

> - add NEWS to %doc (README is currently not worth adding)

There is no NEWS file :-)

> - '%{__mkdir_p} -p' is a duplication. Please don't use macros for simple things
> like %{__cp} or %{__mkdir_p}. You never know if/how these macros are defined.

Fixed

> - use install rather than cp to make sure permissions are correct and to
> preserve timestamps

Used "cp -p" to preserve as its a directory of multiple as per Packaging Guidelines.

> - don't hardcode /usr/share in %install, use %{_datadir}

Fixed

> - Provide the full URL of Source0 and remove the comment on generation of the
> tarball. This will also fix the wrong MD5, but the tarball needs to be build
> and we can't use 'make dist' here. Instead, we do a few steps manually:

Updated the source url. I'll update the spec file with the procedure mentioned above. Actually just wish moblin would actually fix their 'make dist'

> %build
> cd pngs
> ./make.sh
> 
> %install
> rm -rf %{buildroot}
> mkdir -p %{buildroot}/%{_datadir}/icons/moblin/cursors
> for file in xcursors/*; do
>   install -pm 0644 $file %{buildroot}/%{_datadir}/icons/moblin/cursors
> done
> 
> Note that you'll also need 
> BuildRequires:  xorg-x11-apps
> because it provides xcursorgen  

Added.

Comment 6 Christoph Wickert 2009-08-03 22:51:40 UTC
(In reply to comment #5)
> > FAIL - MUST: source does not matche upstream source by MD5
> >   Upstream: 4e88ee79b4aafc08e4dc6defc3dadf7d
> >   Package: 676eef435c190168bf05b51a24f46772
> 
> No its not. Read the note at the top of the spec file about how to recreate the
> tarball.

Yeah, but as I wrote I prefer using one of upstreams tarballs instead of the snapshot. Whatever can happen in the buildsys should happen there without manunally generating a tarball with an unpredictable md5sum.

> > - add NEWS to %doc (README is currently not worth adding)
> 
> There is no NEWS file :-)

Oops, I got confused by the different packages.

> Used "cp -p" to preserve as its a directory of multiple as per Packaging
> Guidelines.

So, where are the files?

Comment 7 Peter Robinson 2009-08-03 22:56:06 UTC
> So, where are the files?  

Getting there.... just doing some testing :-)

Comment 8 Peter Robinson 2009-08-03 23:10:16 UTC
> Yeah, but as I wrote I prefer using one of upstreams tarballs instead of the
> snapshot. Whatever can happen in the buildsys should happen there without
> manunally generating a tarball with an unpredictable md5sum.

Fixed. When I looked closer most of the Makefile was pointless!

Also from this:

> - Summary: 'Moblin X cursors icon theme' should be just 'Moblin X cursors
> theme', because cursors are no icons. On the other hand %decription could be
> more elaborate, e. g.: 'This package contains the Moblin X cursors theme.' or
> similar.

From memory of a discussion on fedora-devel the package descriptions shouldn't contain things like "This package contains" as by definition its a package (and you shouldn't mention the package name in the summary) so I've left it as is for the moment (except the removal of the word 'icon') due to lack of any current inspiration on how to describe cursors :)

So I think all should be addressed here:

SPEC: http://pbrobinson.fedorapeople.org/moblin-cursor-theme.spec
SRPM: http://pbrobinson.fedorapeople.org/moblin-cursor-theme-0.3-2.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1577625

Comment 9 Christoph Wickert 2009-08-03 23:40:50 UTC
(In reply to comment #8)

> From memory of a discussion on fedora-devel the package descriptions shouldn't
> contain things like "This package contains" as by definition its a package (and
> you shouldn't mention the package name in the summary) so I've left it as is
> for the moment (except the removal of the word 'icon') due to lack of any
> current inspiration on how to describe cursors :)

I recall this discussion. Summary should be short, only keywords instead of "foo is a bar...". %description in the other hand can be longer and should contain complete sentences. 


> SPEC: http://pbrobinson.fedorapeople.org/moblin-cursor-theme.spec
> SRPM: http://pbrobinson.fedorapeople.org/moblin-cursor-theme-0.3-2.src.rpm

Let's see what we have:
OK - Summary and description (I still would like the description a little more elaborate, but this is up to you.)
OK - no more disttag.
OK - '%{__mkdir_p} -p'  and %{cp} replacve with simple commands (no blocker anyway)
OK - /usr/share no longer hardcoded
OK - Full URL of Source0 provided
OK - Sources match by md5 4e88ee79b4aafc08e4dc6defc3dadf7d

I still would prefer install over cp, but as the cursors are now being generated during build, their permissions will be fine anyway.

APPROVED

Comment 10 Peter Robinson 2009-08-03 23:46:47 UTC
Thanks!

New Package CVS Request
=======================
Package Name: moblin-cursor-theme
Short Description: Moblin X cursors theme
Owners: pbrobinson
Branches: F-11
InitialCC:

Comment 11 Jason Tibbitts 2009-08-03 23:49:34 UTC
CVS done.

Comment 12 Peter Robinson 2009-08-04 08:52:56 UTC
Built and in rawhide.

Comment 13 Peter Robinson 2009-08-05 10:06:14 UTC
And now in rawhide


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