Bug 331571 - Review Request: xorg-x11-drv-radeonhd - driver for AMD GPG r5xx/r6xx Chipsets
Review Request: xorg-x11-drv-radeonhd - driver for AMD GPG r5xx/r6xx Chipsets
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Thorsten Leemhuis
Fedora Extras Quality Assurance
http://radeonhd.lauft.net/xorg-x11-dr...
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-14 19:49 EDT by Hans Ulrich Niedermann
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-16 17:56:48 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans Ulrich Niedermann 2007-10-14 19:49:52 EDT
Spec URL: http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/xorg-x11-drv-radeonhd.spec
SRPM URL: http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/xorg-x11-drv-radeonhd-1.1.1-0.3.20071014git.fc8.src.rpm
Description: radeonhd driver for AMD GPG r5xx/r6xx Chipsets

This package is still very much a work in progress. You may experience
 * regressions
 * bugs
 * errors
 * broken displays
and other undesirable phenomena.

radeonhd mailing list: http://lists.opensuse.org/radeonhd/

radeonhd source code:
  http://gitweb.freedesktop.org/?p=xorg/driver/xf86-video-radeonhd
Comment 1 Hans Ulrich Niedermann 2007-10-14 19:55:29 EDT
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.
Comment 2 Hans Ulrich Niedermann 2007-10-14 19:58:42 EDT
A "self-review" of the package is contained in
http://radeonhd.lauft.net/xorg-x11-drv-radeonhd/COMMENTS
Comment 3 Hans Ulrich Niedermann 2007-10-15 09:24:45 EDT
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
Comment 4 Thorsten Leemhuis 2007-10-15 10:30:01 EDT
* 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.
Comment 5 Hans Ulrich Niedermann 2007-10-15 10:59:42 EDT
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.
Comment 6 Thorsten Leemhuis 2007-10-15 11:15:43 EDT
(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. 
Comment 7 Hans Ulrich Niedermann 2007-10-15 14:43:21 EDT
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.
Comment 8 Thorsten Leemhuis 2007-10-16 06:52:37 EDT
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?
Comment 9 Hans Ulrich Niedermann 2007-10-16 14:57:35 EDT
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.
Comment 10 Hans Ulrich Niedermann 2007-10-16 14:58:14 EDT
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
Comment 11 Hans Ulrich Niedermann 2007-11-08 17:16:30 EST
Package Change Request
======================
Package Name: xorg-x11-drv-radeonhd
New Branches: F-7 F-8
Comment 12 Kevin Fenzi 2007-11-10 15:33:13 EST
There is already a F-8 branch. 
Added F-7. 

cvs done.
Comment 13 Hans Ulrich Niedermann 2007-11-11 05:08:52 EST
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.
Comment 14 Hans Ulrich Niedermann 2007-11-11 05:11:35 EST
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.
Comment 15 Kevin Fenzi 2007-11-11 12:49:51 EST
cvs done.

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