Bug 235763 - Review Request: windowlab - Small and Simple Amiga-like Window Manager
Summary: Review Request: windowlab - Small and Simple Amiga-like Window Manager
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-04-09 23:39 UTC by Nigel Jones
Modified: 2007-11-30 22:12 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-04-24 05:24:40 UTC
Type: ---
Embargoed:
kevin: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Nigel Jones 2007-04-09 23:39:12 UTC
Spec URL: http://dev.nigelj.com/SRPMS/windowlab.spec
SRPM URL: http://dev.nigelj.com/SRPMS/windowlab-1.34-1.src.rpm
Description: 
WindowLab is a Window Manager for the X Window System. Features include
click-to-focus and a simple menu/taskbar combination. WindowLab is
incredibly fast and small.

rpmlint on src.rpm and built rpm files are clean.  I believe I have got the correct Build-Requires, sadly I don't have a mock enviroment to test with at the moment (I hope to change that soon).

It is also compliant with changes made to the Packaging Guidelines announced today.

n.b> The chmod a-x trick is to keep rpmlint etc happy by not having executable documentation, but technically the whole source should be extracted without executable permissions, my attempt at this failed, so I'd appreciate if someone could shed some light on to it.

Comment 1 Peter Lemenkov 2007-04-10 19:40:35 UTC
I cannot sponsor you so just a few notes:

* windowlab-1.34-pathfixes.patch looks useless - you'd better to redefine $XROOT
variable in Makefile from /usr/X11R6 to /usr - it resolves some other issues.

* change %files section to something like:

%files
%defattr(-,root,root,-)
%dir %{_sysconfdir}/X11/windowlab/
%config(noreplace) %{_sysconfdir}/X11/windowlab/windowlab.menurc
%{_bindir}/windowlab
%{_mandir}/man1/windowlab.1x.gz
%{_datadir}/xsessions/windowlab.desktop
%doc CHANGELOG README TODO LICENCE

Comment 2 Nigel Jones 2007-04-10 22:47:03 UTC
Spec URL: http://dev.nigelj.com/SRPMS/windowlab.spec
SRPM URL: http://dev.nigelj.com/SRPMS/windowlab-1.34-2.src.rpm

-2, updated %files per recommendation, also changed the patch.

Comment 3 Alex Lancaster 2007-04-16 12:29:28 UTC
I also can't sponsor, but I'll make some notes rather than a formal review:

1) Builds cleanly in mock
2) rpmlint is silent on RPM and SRPM
3) Package doesn't respect RPM_OPT_FLAGS, see:

http://fedoraproject.org/wiki/Packaging/Guidelines#head-8b14098227aebff1cf6188939e9d0877295ac448

+ make
gcc -g -O2 -Wall -W -DSHAPE
-DDEF_MENURC="\"/etc/X11/windowlab/windowlab.menurc\"" -I/usr/include  -c main.c
-o m
ain.o
gcc -g -O2 -Wall -W -DSHAPE
-DDEF_MENURC="\"/etc/X11/windowlab/windowlab.menurc\"" -I/usr/include  -c
events.c -o
 events.o


Comment 4 Nigel Jones 2007-04-17 10:55:21 UTC
Spec URL: http://dev.nigelj.com/SRPMS/windowlab.spec
SRPM URL: http://dev.nigelj.com/SRPMS/windowlab-1.34-3.src.rpm

I spoke with Alex on IRC regarding his mock build (while he did it).  It was
then I discovered an issue with the debuginfo package, both have been fixed (the
latter via another patch, depending on upstream's stance the patch may be
merged, but I'd like to keep it seperate for now)

Comment 5 Kevin Fenzi 2007-04-23 01:14:33 UTC
Hey Nigel. I will review this package and look at sponsoring you... 
I know you have 3 other submissions pending, but they are all waiting on
guidelines, correct? Do you have any other packages to submit?

Look for a review of this package in a bit. 

Comment 6 Nigel Jones 2007-04-23 01:40:20 UTC
(In reply to comment #5)
> Hey Nigel. I will review this package and look at sponsoring you... 
> I know you have 3 other submissions pending, but they are all waiting on
> guidelines, correct? Do you have any other packages to submit?
Correct, Bug #235804, Bug #235805 & Bug #235815 are all held up by Ocaml's
static linking, I'm still looking for workarounds on the issue, and considering
asking FESCo at the next meeting to allow an exception (per guidelines).
I'm planning on packaging a game CSP but that is held up by Bug #236517 (current
scons version to too old to build some newer packages.

I'm also still looking at other suitable packages, but once sponsored I'm going
to spend some time to get the ocaml issues sorted with the other 3 packages.
> 
> Look for a review of this package in a bit. 

Will do, thanks.

Comment 7 Kevin Fenzi 2007-04-23 01:51:09 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
d566b989f8e59b169f7affa462762c17  windowlab-1.34.tar
d566b989f8e59b169f7affa462762c17  windowlab-1.34.tar.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.  
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. rpmlint says:

W: windowlab-debuginfo spurious-executable-perm
/usr/src/debug/windowlab-1.34/events.c
W: windowlab-debuginfo spurious-executable-perm
/usr/src/debug/windowlab-1.34/taskbar.c
W: windowlab-debuginfo spurious-executable-perm
/usr/src/debug/windowlab-1.34/menufile.c
W: windowlab-debuginfo spurious-executable-perm /usr/src/debug/windowlab-1.34/main.c
W: windowlab-debuginfo spurious-executable-perm
/usr/src/debug/windowlab-1.34/windowlab.h
W: windowlab-debuginfo spurious-executable-perm
/usr/src/debug/windowlab-1.34/manage.c
W: windowlab-debuginfo spurious-executable-perm /usr/src/debug/windowlab-1.34/new.c
W: windowlab-debuginfo spurious-executable-perm /usr/src/debug/windowlab-1.34/misc.c
W: windowlab-debuginfo spurious-executable-perm
/usr/src/debug/windowlab-1.34/client.c

This is pretty cosmetic, but you could 'chmod 644' those sources and/or
just mention it to the upstream that they shouldn't have executable source files
in their tar.

I don't see any blockers here, so this package is APPROVED. 

I will be happy to sponsor you. 
You can continue the process from: 
http://fedoraproject.org/wiki/PackageMaintainers/Join#head-a601c13b0950a89568deafa65f505b4b58ee869b

Don't forget to close this review request once this package has been imported
and built.


Comment 8 Nigel Jones 2007-04-23 02:46:39 UTC
(In reply to comment #7)
> 1. rpmlint says:
> 
> W: windowlab-debuginfo spurious-executable-perm
> /usr/src/debug/windowlab-1.34/events.c
[...]
> 
> This is pretty cosmetic, but you could 'chmod 644' those sources and/or
> just mention it to the upstream that they shouldn't have executable source files
> in their tar.
> 
Ahh I had that problem before with this package (the chmod trick before, forgot
about debuginfo having the same problem, will fix when possible
> I don't see any blockers here, so this package is APPROVED. 
> 
Thank You
> I will be happy to sponsor you. 
> You can continue the process from: 
>
http://fedoraproject.org/wiki/PackageMaintainers/Join#head-a601c13b0950a89568deafa65f505b4b58ee869b
> 
Thank you again, I have applied for cvsextras/fedorabugs, also could you add me
to the wiki editing group?
> Don't forget to close this review request once this package has been imported
> and built.
> 

New Package CVS Request
=======================
Package Name: windowlab
Short Description: Small and Simple Amiga-like Window Manager
Owners: dev
Branches: FC-5 FC-6
InitialCC: 

Thanks

Comment 9 Nigel Jones 2007-04-24 05:24:40 UTC
Built okay, closing


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