Bug 815951 - Review Request: weston - Reference compositor for Wayland
Review Request: weston - Reference compositor for Wayland
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-24 17:09 EDT by Richard Hughes
Modified: 2012-08-06 01:51 EDT (History)
5 users (show)

See Also:
Fixed In Version: weston-0.89-0.3.fc18
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-04-25 17:36:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Richard Hughes 2012-04-24 17:09:09 EDT
Spec URL: http://people.freedesktop.org/~hughsient/temp/weston.spec
SRPM URL: http://people.freedesktop.org/~hughsient/temp/weston-0.89-0.2.fc17.src.rpm
Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=4020269
Description: Weston is the reference wayland compositor that can run on KMS, under X11 or under another compositor.

rpmlint output:
weston.x86_64: W: spelling-error %description -l en_US wayland -> waylaid, way land, way-land
weston.x86_64: W: no-manual-page-for-binary weston
weston.x86_64: W: no-manual-page-for-binary weston-terminal
weston.x86_64: W: no-manual-page-for-binary weston-launch
weston.src: W: spelling-error %description -l en_US wayland -> waylaid, way land, way-land
weston.src: W: strange-permission make-git-snapshot.sh 0770L
weston.src: W: invalid-url Source0: weston-20120424.tar.bz2
3 packages and 0 specfiles checked; 0 errors, 7 warnings.

Weston has had no tarball releases yet. It'll have a proper 1.0 release in a few months time, but I want people to start developing for wayland in the meantime. I'm planning to produce a wayland-using weston livecd based on F17 this week for people to play with.
Comment 1 Peter Lemenkov 2012-04-25 07:23:27 EDT
I'll review it
Comment 2 Peter Lemenkov 2012-04-25 07:42:35 EDT
Few notes:

* You may drop %clean section entirely. I believe this package isn't intended to run on onl EL boxes.
* Unowned directories
** %{_libdir}/weston/
** %{_libdir}/weston/
Either specifically mark them as %dir in the %files section or change %files section to that

%{_bindir}/weston
%{_bindir}/weston-launch
%{_bindir}/weston-terminal
%{_libdir}/weston/
%{_libexecdir}/weston-*
%{_datadir}/weston/

* I don't like this line
autoreconf -v --install || exit 1
So if autoreconf were fail for whatever reason what would we expect then? Successful building?
Can you simplify this to "autoreconv -ivf" (notice -f switch)?

* License field is wron. Must be "BSD and CC-BY-SA". The latter is for content.

Otherwise it looks fine for me.

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ rpmlint is not silent but these messages are harmless.

work ~/Desktop: rpmlint weston-*
weston.src: W: spelling-error %description -l en_US wayland -> waylaid, way land, way-land
weston.src: W: strange-permission make-git-snapshot.sh 0770L
weston.src: W: invalid-url Source0: weston-20120424.tar.bz2
weston.x86_64: W: spelling-error %description -l en_US wayland -> waylaid, way land, way-land
weston.x86_64: W: no-manual-page-for-binary weston
weston.x86_64: W: no-manual-page-for-binary weston-terminal
weston.x86_64: W: no-manual-page-for-binary weston-launch
3 packages and 0 specfiles checked; 0 errors, 7 warnings.
work ~/Desktop: 

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.

+/- The package meets the Packaging Guidelines except the notes stated above.

+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.

- The License field in the package spec file MUST match the actual license. See above.

- The file, containing the text of the license(s) for the package (or for the part of the package), MUST be included in %doc. Please mark data/COPYRIGHT as %doc.

+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the Source1. 
+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji link above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files in some of the dynamic linker's default paths.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.

- The package MUST own all directories that it creates. See my notes above.

+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.

+ The package has an empty %clean section, which is ok but weird a bit.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain devel-libraries files.
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application in a commot sense (no need to have a *.desktop file)
+ The package does not own files or directories already owned by other packages.
+ All filenames in rpm packages are valid UTF-8.

So, please, address/explain my notes and I'll finish it.
Comment 3 Richard Hughes 2012-04-25 08:03:22 EDT
(In reply to comment #2)
> Few notes:
> 
> * You may drop %clean section entirely. I believe this package isn't intended
> to run on onl EL boxes.

Correct, fixed.

> * Unowned directories
> ** %{_libdir}/weston/
> ** %{_libdir}/weston/
> Either specifically mark them as %dir in the %files section or change %files
> section to that

Fixed, thanks.

> * I don't like this line
> autoreconf -v --install || exit 1
> So if autoreconf were fail for whatever reason what would we expect then?
> Successful building?
> Can you simplify this to "autoreconv -ivf" (notice -f switch)?

Yup, done.

> * License field is wron. Must be "BSD and CC-BY-SA". The latter is for content.

Agreed, fixed.

> So, please, address/explain my notes and I'll finish it.

New files: 
http://people.freedesktop.org/~hughsient/temp/weston.spec
http://people.freedesktop.org/~hughsient/temp/weston-0.89-0.3.fc17.src.rpm

Thanks for the super-quick turnaround.

Richard.
Comment 4 Peter Lemenkov 2012-04-25 08:12:41 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > Few notes:
...
> > * Unowned directories
> > ** %{_libdir}/weston/
> > ** %{_libdir}/weston/
> > Either specifically mark them as %dir in the %files section or change %files
> > section to that
> 
> Fixed, thanks.

This one is still unowned:

%{_libdir}/weston/

Except of that (and assuming that you'll fix it shortly) I don't see any other issues so this package is


APPROVED.
Comment 5 Richard Hughes 2012-04-25 10:12:50 EDT
New Package SCM Request
=======================
Package Name: weston
Short Description: Reference compositor for Wayland
Owners: rhughes
Branches: f17
InitialCC: rhughes
Comment 6 Gwyn Ciesla 2012-04-25 10:21:54 EDT
Git done (by process-git-requests).
Comment 7 Richard Hughes 2012-04-25 17:36:31 EDT
Thanks guys!
Comment 8 Elliott Sales de Andrade 2012-08-06 01:51:58 EDT
Is there a reason this is not also in Fedora 17? The compositor was removed from the wayland package, but this replacement doesn't seem to have appeared there.

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