Bug 175844 - Review Request: wmx -- really simple and basic X window manager
Review Request: wmx -- really simple and basic X window manager
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ignacio Vazquez-Abrams
David Lawrence
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-12-15 12:53 EST by Gabriel Somlo
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-03-16 16:10:24 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Gabriel Somlo 2005-12-15 12:53:37 EST
Spec Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx.spec
SRPM Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx-6pl1-1.src.rpm
Description: wmx is a really simple and basic window manager for the X windowing system.

This is my first package submission, seeking sponsorship.
Comment 1 Peter Lemenkov 2005-12-15 16:50:57 EST
Needs some work:

> Source0: %{name}-%{version}.tar.gz

Use only internet-names, like:

 Source0: http://www.all-day-breakfast.com/wmx/wmx-6.tar.gz

I can't find where can I download examples.

> BuildRoot: %{_tmppath}/%{name}-root

Use the following string:

 BuildRoot: 	%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

> make

It's always a good idea to use the following expression instead of plain "make":

 %{__make} %{?_smp_mflags}

> rm -rf %{buildroot}

Instead, use:

 %{__rm} -rf %{buildroot}

and so on...

You'd better to look at the spec-files which conforms the fedora-extras rules.
You may find 'em  at the:

http://cvs.fedora.redhat.com/viewcvs/devel/?root=extras

Anyway, it's a good idea to include a lightweight wm info FE. 
Comment 2 Peter Lemenkov 2005-12-15 16:52:53 EST
(In reply to comment #1)
> Needs some work:
> 
> > Source0: %{name}-%{version}.tar.gz
> 
> Use only internet-names, like:
> 
>  Source0: http://www.all-day-breakfast.com/wmx/wmx-6.tar.gz

Oops!

 Source0: http://www.all-day-breakfast.com/%{name}/%{name}-%{version}.tar.gz

Comment 3 Ignacio Vazquez-Abrams 2005-12-15 17:24:12 EST
(In reply to comment #1)
> > rm -rf %{buildroot}
> 
> Instead, use:
> 
>  %{__rm} -rf %{buildroot}
> 
> and so on...

Strictly speaking, it's completely optional to use the macros instead of the
commands.

(In reply to comment #2)
> >  Source0: http://www.all-day-breakfast.com/wmx/wmx-6.tar.gz
> 
> Oops!
> 
>  Source0: http://www.all-day-breakfast.com/%{name}/%{name}-%{version}.tar.gz

Actually, macros should only be used in the tarball filename, not in the path.
Comment 4 Gabriel Somlo 2005-12-16 13:35:44 EST
Fixed spec file, bumped release to 2:

Spec Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx.spec
SRPM Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx-6pl1-2.src.rpm

the examples aren't downloaded from anywhere -- I added them to give users a
template on how to build their menu. Anyway, I reworked that: now, menu examples
go into a default systemwide directory (/usr/share/wmx/menu) and the example
xinitrc file goes in %doc.

Thanks for the comments, and let me know what you think of the changes.
Comment 5 Peter Lemenkov 2005-12-16 15:08:17 EST
(In reply to comment #4)

> Spec Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx.spec
> SRPM Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx-6pl1-2.src.rpm

 %{__install} -s -D -m 755 wmx %{buildroot}%{_prefix}/X11R6/bin/wmx

Devel-branch of FC doesn't have /usr/X11R6 and there isn't a particular reason
to use this path if it exists. Change it to:

 %{__install} -s -D -m 755 wmx %{buildroot}%{_bindir}/wmx

You should change %files section in the similar way:

%files
%defattr(-,root,root)
%doc README* UPDATES ANNOYING-BUGS wmx.xsession
%{_bindir}/wmx
%{_datadir}/%{name}

O/T:

Have you looked at the configure-log carefully?

...
checking whether you're still watching... probably not :-)
...

Comment 6 Gabriel Somlo 2005-12-16 16:45:51 EST
Got rid of X11R6, using %{_bindir} instead:

Spec Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx.spec
SRPM Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx-6pl1-3.src.rpm

> checking whether you're still watching... probably not :-)

yeah, I always thought that was rather cute... :)
Comment 7 Jeff Carlson 2005-12-17 15:16:26 EST
I don't think it's necessary to include the package name in either Summary or
%description.  For example,

Summary: a really simple window manager for X

And %description should begin with a capital:  A really simple...

Next, it would be nice to include the appropriate facilities for this window
manager to show up in the xdm list of available sessions.  This can be done by
adding text files to /etc/X11/[g]dm/Sessions.  If it's gdm, the file would
simply be wmx, but if it's dm, it's wmx.desktop.  I'm looking at WindowMaker as
an example:

$ cat /etc/X11/gdm/Sessions/WindowMaker
#!/bin/sh
exec /etc/X11/xdm/Xsession wmaker
$ cat /etc/X11/dm/Sessions/WindowMaker.desktop
[Desktop Entry]
Encoding=UTF-8
Name=WindowMaker
Comment=Start Window Maker
Exec=wmaker
Type=Application

[Window Manager]
SessionManaged=true

Finally, in %files, I prefer to use %{_bindir}/* instead of naming the binary. 
Since this is only one in this case, it's up to you.  If the package ever grows
to include more binaries, it might be nice just to have the wildcard so there is
less to change in the future.  It's just a suggestion.
Comment 8 Gabriel Somlo 2005-12-19 10:25:18 EST
Spec Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx.spec
SRPM Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx-6pl1-4.src.rpm

Fixed 'Summary' and '%description'.

Also, added wmx.desktop. gdm has a defaul search path for .desktop files
(/etc/X11/sessions/:/etc/X11/dm/Sessions/:/usr/share/gdm/BuiltInSessions/:/usr/share/xsessions/)
Now, of these, only the latter two (both in /usr/share/) already existed on my
stock FC4 machine. So, I placed wmx.desktop in /usr/share/xsessions/ along with
the kde and gnome .desktop files which were already there. Why would I go put
them in /etc/X11/dm/Sessions/ instead ? Is the /usr/share/xsessions/ location
being deprecated?

Also, looking at WindowMaker's example, I don't get the purpose of the shell
script that goes in /etc/X11/gdm/Sessions/WindowMaker; where would that get
called from ? Is it supposed to be a wrapper for the wmaker binary called
by gdm via the .desktop file, or ???

Thanks,
Gabriel
Comment 9 Gabriel Somlo 2005-12-19 15:58:03 EST
Spec Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx.spec
SRPM Name or Url: http://www.contrib.andrew.cmu.edu/~somlo/wmx-6pl1-5.src.rpm

If the script /etc/X11/xinit/Xclients.d/Xclients.wmx.sh exists, gdm will use it
to fire up wmx if wmx was selected by the user at the gdm greeting screen. This
latest version supplies such a file, which either finds and fires up a custom set
of apps in the user's home directory, or starts up a few defaults (xterm and
xscreensaver) before launching wmx. Tested it on a stock FC4 box and it looks
like it's working right

Let me know what you all think.
Comment 10 Ignacio Vazquez-Abrams 2005-12-27 01:48:25 EST
- Patches shouldn't use macros in their naming
- The default for %setup -n already is %{name}-%{version}
- Use install -p for %{SOURCE2} instead of cp
- The summary's first letter should be capitalized
- The changelog version/release does not match the package version/release
(6pl1.5 vs. 6pl1-5)
- The package should not own /etc/X11
Comment 12 Ignacio Vazquez-Abrams 2005-12-30 04:16:31 EST
- Missing the appropriate BuildRequires for X
Comment 14 Ignacio Vazquez-Abrams 2005-12-31 21:19:28 EST
- Upstream source matches
- Builds clean on mock in FC4
- Runs on FC4
- Everything else looks good

APPROVED
Comment 15 Ignacio Vazquez-Abrams 2006-02-17 01:00:14 EST
Built under FC4 and devel, but the sponsorship process isn't complete.
Comment 16 Gabriel Somlo 2006-03-16 16:10:24 EST
sponsorship now complete, closing.

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