Bug 426753

Hi, if you do not need to be sponsored, I would like to review this package.

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

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.

Created attachment 314788 [details]
xmonad.spec.patch-1

cleanup and simplification

I am tempted to drop hsc_name too, it just seems to make everything more verbose.
I suggest just changing %{hsc_name} to ghc.

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.

The templates should be made self-contained by including the necessary macros.

Theoretically yeah, but these macros are going to go in the GHC package, based on advice from the Packaging Committee.

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.

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.

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.

Should xmonad not require ghc-xmonad?

Created attachment 316173 [details]
xmonad.spec

an updated spec file that builds with the latest macros in bug 460304

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.)

Ping?

I can update the submission but we need to get ghc-X11 reviewed first.

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.

New update

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

Created attachment 333495 [details]
xmonad.spec-2.patch

cabal2spec sync and some other requires tweaks.

This also build, installs and uninstalls for me.

New update

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

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.

(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.)

$ 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.

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.

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.

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

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

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.

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. :)

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)

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

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.

BTW how about building ghc-X11 for F10 - it would make testing easier for more people I guess.

(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.

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.

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.

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?

> (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

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.

(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/

(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!

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.

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!

(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.

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.

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.

Created attachment 342410 [details]
xmonad-final.patch

I suggest making these small changes before importing to cvs.

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

cvs done.

Built on rawhide and Fedora 11

http://koji.fedoraproject.org/koji/buildinfo?buildID=101104
http://koji.fedoraproject.org/koji/taskinfo?taskID=1339359

Thanks, F11 build needs to be tweaked or wait for ghc-rpm-macros.

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.

New Package CVS Request
=======================
Package Name: xmonad
Short Description: a tiling window manager written in haskell
Owners: mathstuf
Branches: el6
InitialCC: haskell-sig

Missed the flag.

You mean a package change request here right? 
Not a new package?

Package Change Request
=======================
Package Name: xmonad
New Branches: el6
Owners: mathstuf
InitialCC: haskell-sig

Git done (by process-git-requests).