Bug 426753 - Review Request: xmonad - A tiling window manager
Review Request: xmonad - A tiling window manager
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jens Petersen
Fedora Extras Quality Assurance
:
Depends On: ghc-X11
Blocks: 426754
  Show dependency treegraph
 
Reported: 2007-12-25 15:18 EST by Yaakov Nemoy
Modified: 2010-10-10 00:19 EDT (History)
13 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-06 18:03:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
petersen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
xmonad.spec.patch-1 (6.42 KB, patch)
2008-08-22 05:39 EDT, Jens Petersen
no flags Details | Diff
xmonad.spec (4.24 KB, text/plain)
2008-09-09 07:05 EDT, Jens Petersen
no flags Details
xmonad.spec-2.patch (2.76 KB, patch)
2009-02-27 10:29 EST, Jens Petersen
no flags Details | Diff
add desktop file, package manpage, sample config and other docs (1.92 KB, patch)
2009-03-30 16:48 EDT, Till Maas
no flags Details | Diff
xmonad-final.patch (1.96 KB, patch)
2009-05-05 00:09 EDT, Jens Petersen
no flags Details | Diff

  None (edit)
Description Yaakov Nemoy 2007-12-25 15:18:38 EST
Spec URL: http://fedorapeople.org/~ynemoy/xmonad/xmonad.spec
SRPM URL: http://fedorapeople.org/~ynemoy/xmonad/xmonad-0.5-1.fc8.src.rpm
Description: xmonad is a tiling window manager for X. Windows are arranged
automatically to tile the screen without gaps or overlap, maximising
screen use. All features of the window manager are accessible from
the keyboard: a mouse is strictly optional. xmonad is written and
extensible in Haskell. Custom layout algorithms, and other
extensions, may be written by the user in config files. Layouts are
applied dynamically, and different layouts may be used on each
workspace. Xinerama is fully supported, allowing windows to be tiled
on several screens.
Comment 1 Adam Goode 2008-02-12 18:18:39 EST
Hi, if you do not need to be sponsored, I would like to review this package.
Comment 2 Jens Petersen 2008-02-12 21:49:10 EST
Note this package requires ghc-X11 which is not yet in Fedora,
but see bug 351361.

BTW I have some (newer) packages which I made independently (probably need
some cleanup for Fedora):

http://haskell.org/fedora/haskell/SRPMS/xmonad-0.6-1.src.rpm
http://haskell.org/fedora/haskell/SRPMS/ghc-X11-1.4.1-1.src.rpm
Comment 3 Yaakov Nemoy 2008-07-03 06:18:36 EDT
SPEC: http://ynemoy.fedorapeople.org/repo/xmonad.spec
SRPM: http://ynemoy.fedorapeople.org/repo/xmonad-0.7-1.fc9.src.rpm

This update follows the Draft Guidelines, which are under review.  Reviewing
this package will help find problems in the guidelines.
Comment 4 Jens Petersen 2008-08-22 05:39:53 EDT
Created attachment 314788 [details]
xmonad.spec.patch-1

cleanup and simplification
Comment 5 Jens Petersen 2008-08-22 05:44:34 EDT
I am tempted to drop hsc_name too, it just seems to make everything more verbose.
I suggest just changing %{hsc_name} to ghc.
Comment 6 Yaakov Nemoy 2008-08-26 20:09:18 EDT
I've redone the guidelines completely to use macros.  I'll be going over this in the coming weeks to make some badly needed updates to everything.
Comment 7 Jens Petersen 2008-08-27 03:16:05 EDT
The templates should be made self-contained by including the necessary macros.
Comment 8 Yaakov Nemoy 2008-08-27 09:45:28 EDT
Theoretically yeah, but these macros are going to go in the GHC package, based on advice from the Packaging Committee.
Comment 9 Jason Tibbitts 2008-08-27 10:05:25 EDT
The guidelines were approved (and hopefully will be accepted by FESCo today) but of course we need to see a ghc package rev with those macros included before reviews can really move forward.  I guess this package (and perhaps others) will also need an update to conform to the new prettified specfule template.
Comment 10 Yaakov Nemoy 2008-09-02 23:07:39 EDT
SPEC: http://ynemoy.fedorapeople.org/repo/ghc-X11.spec
SRPM: http://ynemoy.fedorapeople.org/repo/ghc-X11-1.4.2-1.fc9.src.rpm

This makes it compliant with the guidelines.

This is an example of a package that dynamicaly links to C libraries and is only a library.

The macros needed are currently available on the wiki and need to be installed manually for now.  There is a bug to have them included in ghc directly.  I will add it to the blockers.  Don't forget to --copyin them to mock, and then use --no-clean to do the build.
Comment 11 Yaakov Nemoy 2008-09-02 23:10:18 EDT
I apologize, that's the wrong package.  Here are the correct links.
SPEC: http://ynemoy.fedorapeople.org/repo/xmonad.spec
SRPM: http://ynemoy.fedorapeople.org/repo/xmonad-0.7-3.fc9.src.rpm


This is an example of a binary and library package.
Comment 12 Jens Petersen 2008-09-09 06:24:21 EDT
Should xmonad not require ghc-xmonad?
Comment 13 Jens Petersen 2008-09-09 07:05:21 EDT
Created attachment 316173 [details]
xmonad.spec

an updated spec file that builds with the latest macros in bug 460304
Comment 14 Yaakov Nemoy 2008-09-09 20:53:21 EDT
a) turns out there's a new xmonad release.  I've even built packages for it.  I'll test the new macros, and rebuild everything.

b) xmonad generally requires ghc-xmonad.  there are very very few cases where it doesn't and that's not the normal use case.  since we don't have a 'highly-suggested-install-this-or-die' category like debian, i'm going to acommodate 99% of use cases and note that xmonad definitely requires ghc-xmonad (although technically the other way around isn't necessary.)
Comment 15 Thomas Moschny 2008-11-27 03:11:42 EST
Ping?
Comment 16 Jens Petersen 2008-11-27 19:24:30 EST
I can update the submission but we need to get ghc-X11 reviewed first.
Comment 17 Yaakov Nemoy 2009-01-21 22:34:04 EST
Let's pick this up again.
SPEC: http://ynemoy.fedorapeople.org/review/xmonad.spec
SRPM: http://ynemoy.fedorapeople.org/review/xmonad-0.8.1-1.fc11.src.rpm

Currently, this only compiles on rawhide, so get your rawhide builders ready.
Comment 19 Jens Petersen 2009-02-27 10:29:06 EST
Created attachment 333495 [details]
xmonad.spec-2.patch

cabal2spec sync and some other requires tweaks.

This also build, installs and uninstalls for me.
Comment 21 Jens Petersen 2009-03-10 07:45:29 EDT
I don't know if the ghc-X11 = 1.4.5 is really necessary?
Well according to hackage it requires X11 (>=1.4.3),
so I suggest using >= 1.4.3 unless there is a good reason not to.

BSD3 is called BSD in Fedora.

Otherwise the cabal2spec-diff looks good to me.
Comment 22 Jens Petersen 2009-03-10 07:49:24 EDT
(Arguably the xmonad requires of ghc is redundant since it requires
ghc-xmonad-devel, which requires ghc anyway, but I guess it doesn't
really matter either way: I suppose it further reminds one that xmonad
may recompile itself in userspace when customized.)
Comment 23 Yaakov Nemoy 2009-03-13 18:59:54 EDT
$ python
>>> import this
<snip>
Explicit is better than implicit.
<snip>

Anyways, we need to include ghc-xmonad-devel because xmonad can and will recompile itself on the fly.
Comment 24 Yaakov Nemoy 2009-03-13 19:40:18 EDT
New update

SPEC: http://ynemoy.fedorapeople.org/review/xmonad.spec
SRPM: http://ynemoy.fedorapeople.org/review/xmonad-0.8.1-4.fc10.src.rpm  

Arguably, this package needs to be rebuilt everytime there's an update to X11, because of static compilation. Still, there is one use case for giving a more generic required version of ghc-X11, and that is the user who needs to recompile to an earlier version for what ever reason.

Note that fileperms was fixed too. This is an issue that seems to be common to haskell things compiled in ghc, so we may want to fix this in the template.
Comment 25 Jens Petersen 2009-03-17 04:47:16 EDT
Thanks for the update.

> Note that fileperms was fixed too. This is an issue that seems to be common to
> haskell things compiled in ghc, so we may want to fix this in the template.  

You need to do that just for the bin file like I did with hscolour, not for all files, like this:

%files
%defattr(-,root,root,-)
%doc LICENSE
%attr(755,root,root) %{_bindir}/%{name}

Otherwise looks ok to me: I would like test it in rawhide and then do the final review.
Comment 26 Yaakov Nemoy 2009-03-17 12:33:40 EDT
New update

SPEC: http://ynemoy.fedorapeople.org/review/xmonad.spec
SRPM: http://ynemoy.fedorapeople.org/review/xmonad-0.8.1-5.fc10.src.rpm 

Note to self defattr != attr :P
Comment 27 Till Maas 2009-03-22 12:10:55 EDT
The manpage and the example config file (xmonad.hs) from the directory "man" are not packaged. Also it seems to me that a xmonad.desktop file in /usr/share/xsessions is missing, which allows one to select xmonad as wm in gdm. I used this the following one with an older xmonad release, maybe it works with the current one, too:

http://till.fedorapeople.org/files/xmonad.desktop
Comment 28 Till Maas 2009-03-30 16:48:15 EDT
Created attachment 337244 [details]
add desktop file, package manpage, sample config and other docs

The xmonad.desktop works for me with gdm, but xmonad needs a config file in ~/.xmonad/xmonad.hs to even run. Maybe a wrapper should be added that creates such a config file if there is none. This config file could make xmonad display its manpage per default when it is started to help users new to xmonad. This was what wmii did iirc and I found it pretty helpful back then. I will write such a wrapper if it will be packaged.
Comment 29 Yaakov Nemoy 2009-03-31 23:40:13 EDT
New update

SPEC: http://ynemoy.fedorapeople.org/review/xmonad.spec
SRPM: http://ynemoy.fedorapeople.org/review/xmonad-0.8.1-7.fc10.src.rpm 

This includes both a start-xmonad and an xmonad-session script. The former will check for a config file or copy a config file over. The latter will call a user's .xsession and then start start-xmonad.  There are also .desktop files for both, and both should be installed in the correct location.  Let me know if this works for you.

Currently, the default config is the default config provided by upstream. Should we convince upstream to have it load the manpage on startup?

If you still want to write a script or two, please do, and if it brings any new features / works better than mine, i will be more than happy to include them. :)
Comment 30 Bryan O'Sullivan 2009-03-31 23:48:19 EDT
Spec file is broken:

+ install -p -m 0755 -D /home/bos/rpmbuild/SOURCES/start-xmonad /usr/bin/start-xmonad
install: cannot create regular file `/usr/bin/start-xmonad': Permission denied
error: Bad exit status from /var/tmp/rpm-tmp.sfbpdY (%install)
Comment 31 Bryan O'Sullivan 2009-03-31 23:52:28 EDT
With that simple edit made, I get another error:

Processing files: ghc-xmonad-devel-0.8.1-7.fc10
error: Could not open %files file /home/bos/rpmbuild/BUILD/xmonad-0.8.1/ghc-xmonad-devel.files: No such file or directory
Comment 32 Till Maas 2009-04-01 02:59:20 EDT
I will try to test it later today, but I already found some additional issues:

- There are now tabs used after Source2 to Source4
- start-xmonad probably needs a "exec xmonad" at the end to make it actually start xmonad
- start-xmonad will not find the config file, because there is a doc missing in the path: /usr/share/xmonad-0.8.1/xmonad.hs, nevertheless the sample config should probably copied to /usr/share/xmonad/xmonad.hs, too and not marked as %doc, otherwise the package will not work installed with --nodocs.
Comment 33 Jens Petersen 2009-04-02 11:24:31 EDT
BTW how about building ghc-X11 for F10 - it would make testing easier for more people I guess.
Comment 34 Till Maas 2009-04-02 13:14:36 EDT
(In reply to comment #29)

> This includes both a start-xmonad and an xmonad-session script. The former will
> check for a config file or copy a config file over. The latter will call a
> user's .xsession and then start start-xmonad.  There are also .desktop files
> for both, and both should be installed in the correct location.  Let me know if
> this works for you.

I did not ttest the xsession stuff, but with my patch it works:
http://till.fedorapeople.org/files/xmonad_7-8.spec.diff
I wrote a simple config that is stored in /etc/skel/.xmonad/xmonad.hs, that opens the manpage using xterm.
 
> Currently, the default config is the default config provided by upstream.
> Should we convince upstream to have it load the manpage on startup?

If this can be done easily within xmonad, why not. But I guess if xmonad would open the manpage by default, there would be no easy way to stop it doing this using a xmonad.hs currently.
Comment 35 Yaakov Nemoy 2009-04-02 14:55:28 EDT
ghc-X11 won't build on F-10, i think because of macro errors. We'll need an updated ghc for this.

Meanwhile, users can just pull packages from rawhide.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1274046

@Till, i'll have a look at your patches later.
Comment 36 Jens Petersen 2009-04-04 00:02:17 EDT
Yaakov built it

http://koji.fedoraproject.org/koji/buildinfo?buildID=96455

unfortunately now it has higher release number than rawhide which is not good.

You should use ghc-X11-1.4.5-5.fc10.1 to bump in F-10 to keep it less than ghc-X11-1.4.5-5.fc11.
Comment 37 Jens Petersen 2009-04-06 04:49:37 EDT
Thanks for bumping the f11 ghc-X11 build, Yaakov.

(In reply to comment #34)
> I did not test the xsession stuff

Can't it just be merged into xmonad-start?

> but with my patch it works:
> http://till.fedorapeople.org/files/xmonad_7-8.spec.diff

Thanks

> I wrote a simple config that is stored in /etc/skel/.xmonad/xmonad.hs, that
> opens the manpage using xterm.

Could you attach it here?
Comment 38 Till Maas 2009-04-06 15:11:28 EDT
> (In reply to comment #34)
> > I did not test the xsession stuff
> 
> Can't it just be merged into xmonad-start?

No, today I found a .xsession I used with an older release of xmonad I guess. It contained a exec xmonad.

Btw. a different feature for xmonad-start I want to suggest is to test whether xmonad.hs is newer than the binary and in case it is, recompile it. Or is this something xmonad already does? But it seems it does not recompile itself, when I hit ALT-q.
 
> > but with my patch it works:
> > http://till.fedorapeople.org/files/xmonad_7-8.spec.diff
> 
> Thanks
> 
> > I wrote a simple config that is stored in /etc/skel/.xmonad/xmonad.hs, that
> > opens the manpage using xterm.
> 
> Could you attach it here?  

I meant to include it in the patch, otherwise the patch is not functional. I uploaded a new version of the patch to above URL, but the config file is now also available at:
http://till.fedorapeople.org/files/xmonad.hs.xterm_manpage.hs
Comment 39 Bryan O'Sullivan 2009-04-07 02:35:20 EDT
Can you post complete spec files instead of patches? It's too hard to track what's a patch to a spec file, vs a patch to the source.
Comment 40 Till Maas 2009-04-07 02:49:35 EDT
(In reply to comment #39)
> Can you post complete spec files instead of patches? It's too hard to track
> what's a patch to a spec file, vs a patch to the source.  

http://till.fedorapeople.org/files/xmonad-0.8.1-8.fc10.src/
Comment 41 Jens Petersen 2009-04-15 05:34:53 EDT
(In reply to comment #38)
> Btw. a different feature for xmonad-start I want to suggest is to test whether
> xmonad.hs is newer than the binary and in case it is, recompile it. Or is this
> something xmonad already does?

I believe it should but I am a bit rusty on xmonad.

> But it seems it does not recompile itself, when
> I hit ALT-q.

That also work from what I was reading on their wiki.

> I meant to include it in the patch, otherwise the patch is not functional. I
> uploaded a new version of the patch to above URL, but the config file is now
> also available

Thanks - will try that out!
Comment 42 Jens Petersen 2009-04-25 09:53:22 EDT
http://petersen.fedorapeople.org/xmonad/xmonad.spec
http://petersen.fedorapeople.org/xmonad/xmonad-0.8.1-9.fc10.src.rpm

I have been using this package now recently and it works ok for me -
uses most of Till's latest package but slightly simpler:
just xmonad.desktop for xmonad-start.

I feel this is probably enough for the initial fedora xmonad package.
We can get more sophisticated later if necessary, but for now
I suggest advanced users who want to customize their session startup
can use xorg-x11-xinit-session and ~/.xsession say.
Comment 43 Yaakov Nemoy 2009-04-27 21:30:23 EDT
New update

SPEC: http://ynemoy.fedorapeople.org/review/xmonad.spec
SRPM: http://ynemoy.fedorapeople.org/review/xmonad-0.8.1-11.fc10.src.rpm

This does a few new things.

1) carries around a ppc workaround that really should go in macros.ghc
2) only builds on fedora 12 (yay?) so you need koji to make this work for you
http://koji.fedoraproject.org/koji/taskinfo?taskID=1325396
3) converts the value added default config into a patch directly against the upstream source. what this patch does is replaces the xmonad line of the default verbose config with an expansion that loads xterm with the man page showing. in the future, i'll probably add a comment to the user
4) includes just xmonad-start without doing xmonad-session.
5) renumbers the source to make more sense
6) includes other changes proposed here

enjoy!
Comment 44 Jens Petersen 2009-05-04 00:03:40 EDT
(In reply to comment #43)
> New update

Thanks!

> 1) carries around a ppc workaround that really should go in macros.ghc

Yeah we could add something in the macros to help with this, but it is a bit tedious: I hope we can fix the real problem on ppc soon.

> 2) only builds on fedora 12 (yay?)

I hope to backport latest ghc to f11 soon.  We might have to wait for a new gtk2hs release.

> 3) converts the value added default config into a patch directly against the
> upstream source. what this patch does is replaces the xmonad line of the
> default verbose config with an expansion that loads xterm with the man page
> showing. in the future, i'll probably add a comment to the user

Good idea.

I think better to rename the patch to xmonad-config-show-manpage.patch
or something like that.

> 4) includes just xmonad-start without doing xmonad-session.

Good - I think the name in the .desktop file should just be "xmonad" or "XMonad" though not "xmonad-start".

> 5) renumbers the source to make more sense
> 6) includes other changes proposed here

Ok

Otherwise looks pretty good to me now.  I am going to take it for a spin and then do the review.
Comment 45 Jens Petersen 2009-05-04 23:58:51 EDT
I tested the scratch build on f11 with f12 ghc and it works fine for me.
I will attach a patch for the suggestions above after this.
Comment 46 Jens Petersen 2009-05-05 00:00:45 EDT
Here is my formal review.


 +:ok, !:needs attention, -:needs fixing,  NA: not applicable

MUST Items:
[!] MUST: rpmlint output

Please fix the tabs that appeared spec file for the patch.

Also fixed in my next patch.

[+] MUST: Package Naming Guidelines
[+] MUST: spec file name must match base package %{name}
[+] MUST: Packaging Guidelines.

Follows Haskell Packaging Guidelines

[+] MUST: Licensing Guidelines
[+] MUST: License field in the package spec file must match actual license.
[+] MUST: include license files in %doc if available in source
[+] MUST: The spec file must be written in American English and be legible.
[+] MUST: source md5sum matches upstream release

03a8f0a420902d9eea3df1d8d62598c7  xmonad-0.8.1.tar.gz

[+] MUST: must successfully compile and build into binary rpms on one main arch
[+] MUST: if necessary use ExcludeArch for other archs
[+] MUST: All build dependencies must be listed in BuildRequires
[NA] MUST: use %find_lang macro for .po translations
[NA] MUST: packages which store shared library files in the dynamic linker's default paths, must call ldconfig in %post and %postun.
[NA] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review
[+] MUST: A package must own all directories that it creates.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example.
[+] MUST: Each package must have a %clean section
[+] MUST: Each package must consistently use macros, as described in the macros section of Packaging Guidelines.
[+] MUST: The package must contain code, or permissable content.
[+] MUST: Large documentation files should go in a doc subpackage.
[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application.
[+] MUST: Header files must be in a -devel package.
[NA] MUST: Static libraries must be in a -static package.
[NA] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).
[NA] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
[NA] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
[NA] MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.


Package is APPROVED.
Comment 47 Jens Petersen 2009-05-05 00:09:31 EDT
Created attachment 342410 [details]
xmonad-final.patch

I suggest making these small changes before importing to cvs.
Comment 48 Yaakov Nemoy 2009-05-06 13:58:23 EDT
New Package CVS Request
=======================
Package Name: xmonad
Short Description: a tiling window manager written in haskell
Owners: ynemoy
Branches: F-11
InitialCC: fedora-haskell-sig
Comment 49 Kevin Fenzi 2009-05-06 16:56:08 EDT
cvs done.
Comment 51 Jens Petersen 2009-05-06 20:26:58 EDT
Thanks, F11 build needs to be tweaked or wait for ghc-rpm-macros.
Comment 52 Yaakov Nemoy 2009-05-06 20:37:56 EDT
Yeah, i was gonna do that in one fell swoop. If someone could finish that review, it'll mean i won't have to bother putting in a temporary fix in the xmonad-fc11 package.
Comment 53 Ben Boeckel 2010-10-07 00:18:28 EDT
New Package CVS Request
=======================
Package Name: xmonad
Short Description: a tiling window manager written in haskell
Owners: mathstuf
Branches: el6
InitialCC: haskell-sig
Comment 54 Ben Boeckel 2010-10-07 00:18:48 EDT
Missed the flag.
Comment 55 Kevin Fenzi 2010-10-08 16:25:34 EDT
You mean a package change request here right? 
Not a new package?
Comment 56 Ben Boeckel 2010-10-08 17:33:57 EDT
Package Change Request
=======================
Package Name: xmonad
New Branches: el6
Owners: mathstuf
InitialCC: haskell-sig
Comment 57 Kevin Fenzi 2010-10-10 00:19:32 EDT
Git done (by process-git-requests).

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