Bug 331571

On my machine, I have three options to run X11:

  1. svga driver (open source, sloooow, leaves text console unusable)
  2. fglrx driver (closed source, 3D support, mostly hangs on suspend/resume)
  3. radeonhd (open source, 2D only, mostly survives suspend/resume)

Given radeonhd's current status of not even having had the first release, this
package would probably not be a good candidate to be shipped on Fedora CDs for
some time to come.

A "self-review" of the package is contained in
http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/COMMENTS

I have just uploaded an updated package for the current git snapshots, as that
requires less patches than yesterday.

Index: http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/
SPEC: http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/xorg-x11-drv-radeonhd.spec
SRPM:
http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/xorg-x11-drv-radeonhd-1.1.1-0.4.20071015git.fc8.src.rpm

* driverdir defined, but not used (there are two places where it could be used); 

* why "%define date %(TZ=UTC date +%Y%m%d)"? That will produce a incorrect
alphatag tomorrow (e.g. according to release it will be 20071016, but the
sources are still those from 20071015)

* you shound include the xorg-x11-drv-radeonhd-snapshot.sh script

* why "Version: 1.1.1"?
[...]
(II) Module radeonhd: vendor="AMD GPG"
        compiled for 1.3.0, module version = 0.0.1
[...]

* is "BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)"
officially accepted these days? Looks to me like
http://fedoraproject.org/wiki/PackagingDrafts/BuildRoot is still a draft...

* I'd say trim the description and put"
 * regressions
 * bugs
 * errors
 * broken displays
" in one line; I'd also say that the link to the source code doesn't make much
sense in the description

* why the "-n xorg-x11-drv-radeonhd" for %files?

* don't use ".fc8" in changelog, as rpmlint will complain if one builds the
package on F7

* "ExcludeArch:   s390 s390x" not actually needed, but won't hurt

Looks fine otherwise.

Thanks for the review. I'll address all your points here:

* Removed driverdir definition.

* Any timezone is arbitrary. I chose to use the canonical timezone instead of
"the one the current packager happens to reside in". Is that wrong?

* Why Version: 1.1.1? Because the upstream package says so. I am going to take
that upstream. My guess is this will change to 0.0.1.

* Buildroot definition:
 
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473
  I quote: "If unsure, simply pick [the one with mktemp]."
  That looks to me like "officially accepted".

* Description shortened.

* %files now without -n

* Removed .fc8 from %changelog entries.

* "ExcludeArch: s390 s390x" was copied from xorg-x11-drv-ati, so I guessed it
made sense. I think I'll keep it.

The new 0.5.20071015git package addresses these things:
http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/xorg-x11-drv-radeonhd.spec
http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/xorg-x11-drv-radeonhd-1.1.1-0.5.20071015git.fc8.src.rpm

I will upload a 0.6 when I have talked with upstream about Version: 1.1.1 vs. 0.0.1.

(In reply to comment #5)
> * Any timezone is arbitrary. I chose to use the canonical timezone instead of
> "the one the current packager happens to reside in". Is that wrong?

Not the timezone itself is the problem -- that "date +%Y%m%d" is executed each
time when the rpms get build. Thus when I rebuild
xorg-x11-drv-radeonhd-1.1.1-0.5.20071015git.fc8.<foo>.rpm today it will stick to
that name; but if I rebuild it in 24hours from now I'll afaics will get 
xorg-x11-drv-radeonhd-1.1.1-0.5.20071016git.fc8.<foo>.rpm -- but that's
misleading, is still contains the snapshot from 20071015. You need to hardcode
the version. 

Ah, right.

So... we need a static date in the spec file, which must be consistent with the
time when the snapshot was baked into a tarball.

Is having the snapshot script update the "%define date" line of the spec file
after successful generation of a snapshot tarball considered bad style? :-)

It now is "sh foo-snapshot.sh --update-spec".

Also, the snapshot tarballs are now called something like
"xf86-video-radeonhd-0.0.1-20071015git.tar.bz2".

http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/xorg-x11-drv-radeonhd.spec
http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/xorg-x11-drv-radeonhd-0.0.1-0.6.20071015git.fc8.src.rpm

The version is hardcoded at 0.0.1 now. Upstream has two build systems (automake
and imake/xmkmf), so they can't just set the version according to configure.ac.
Anyway, upstream cannot go much below 0.0.1 for too long, so we can use our
hardcoded 0.0.1 for the time being.

Looks good now; rpmlint warns:

xorg-x11-drv-radeonhd-debuginfo.x86_64: W: filename-too-long-for-joliet
xorg-x11-drv-radeonhd-debuginfo-0.0.1-0.6.20071015git.fc7.x86_64.rpm

But that can be ignored. 

APPROVED

Just one final question -- what about the "conntest" tool? Shouldn't we ship
that as additional package in case users want to help the developers? Or should
those users build it own their own?

Thanks for review and approval.

Upstream argued not to ship conntest.

I'd argue that in the current state of affairs, running conntest only makes
sense if you really run the very latest version - i.e. directly from git.

New Package CVS Request
=======================
Package Name: xorg-x11-drv-radeonhd
Short Description: Xorg X11 radeonhd driver for AMD GPG r5xx/r6xx Chipsets
Owners: ndim,ajax
Branches: 
InitialCC: 
Cvsextras Commits: no

Package Change Request
======================
Package Name: xorg-x11-drv-radeonhd
New Branches: F-7 F-8

There is already a F-8 branch. 
Added F-7. 

cvs done.

Argh sorry. I had wrongly presumed a "cvs up" would check out all branches.

Sorry 2... I had wrongly presumed that the ACL would be copied in the creation
of the branch.

New cvsadmin request coming up.

Package Change Request
======================
Package Name: xorg-x11-drv-radeonhd
Branches: F-7
Updated Fedora Owners: ndim,ajax

According to
https://admin.fedoraproject.org/pkgdb/packages/name/xorg-x11-drv-radeonhd ajax
needs to be added to the F-7 branch. F-8 and devel are fine.

cvs done.