Bug 433738 - Review Request: xf86-video-nouveau - X.org nouveau driver
Summary: Review Request: xf86-video-nouveau - X.org nouveau driver
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-02-21 03:28 UTC by Dave Airlie
Modified: 2008-05-17 18:02 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-05-17 18:02:45 UTC
Type: ---
Embargoed:
j: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Dave Airlie 2008-02-21 03:28:55 UTC
Spec URL: http://people.fedoraproject.org/~airlied/nouveau/xorg-x11-drv-nouveau.spec
SRPM URL: http://people.fedoraproject.org/~airlied/nouveau/xorg-x11-drv-nouveau-0.0.10-1.fc9.src.rpm
Description: Reverse Engineered Xorg X11 driver for nvidia hardware provided by the nouveau project.

Comment 1 Jason Tibbitts 2008-02-21 22:10:49 UTC
Builds OK; rpmlint says:

  xorg-x11-drv-nouveau.src: W: mixed-use-of-spaces-and-tabs
   (spaces: line 9, tab: line 3)
which I don't really care about, fix it if you like.

   xorg-x11-drv-nouveau.x86_64: W: incoherent-version-in-changelog
    2.1.5-2 1:0.0.10-1.fc9
You probably just want to start afresh with your changelog, but if not you
should at least have an entry for the current revision.

Could you provide instructions for generating the tarball in use?  Just a coupe
of comments for checking out a specific git tag from the repository should be
fine; see http://fedoraproject.org/wiki/Packaging/SourceURL for more info.

Is this actually version 0.0.10 or is it a snapshot from the source tree made
after 0.0.10 was released?  If the latter, we have specific guidelines relating
to the versioning of snapshots, which would indicate that this should be
0.0.10-1.20080221 or 0.0.10-1.20080221git (what follows the date is up to you;
some use an abbreviated commit ID).

Have to run now; more review later.

Comment 2 Dave Airlie 2008-02-22 02:19:33 UTC
okay I've fixed the changelog, and versioning and tab char...

so the new stuff is at the same URL.

xorg-x11-drv-nouveau-0.0.10-0.20080221git5db7920.fc9.src.rpm
is new src rpm name

and I've updated the specfile to build that from the tar ball, and added
instructions for git cloning it. The upstream project hasn't yet done a release,
the version is based off the kernel API they use and report in the log files.



Comment 3 Jason Tibbitts 2008-02-22 23:51:02 UTC
I have to admit that my git knowledge is a bit shallow, but I'm a bit confused
as to how you know what git revision to checkout to generate the tarball when
you don't have the tarball from which to extract the git revision.

Also, I don't think tare working quite properly; when the package is built,
you can't directly access %{tarfile} and even though it's a build dependency,
git-core isn't installed in the buildroot when the srpm is built.  So the end
the package gets a VR of just 0.0.10-0.20080221git.fc9 and you get this in the
build log:

sh: git-get-tar-commit-id: command not found
bzip2: Can't open input file xf86-video-nouveau-0.0.10-20080221.tar.bz2: No
  such file or directory.

For simplicity, I'd just suggest hardcoding the git_version, or even just
dropping it from the release (it's in no way mandatory) and just sticking it
in the instructional comments.

Once I extracted the commit-id from the tarball manually, I was able to follow
the comments in the spec to recreate the archive and verify that it matched,
although your instructions are missing a "cd xf86-video-nouveau".

Now, there are some new rpmlint complaints:

  xorg-x11-drv-nouveau.x86_64: W: incoherent-version-in-changelog 0.0.10-1 
   1:0.0.10-0.20080221git.fc9
See below; basically you want your changelog entry to match the actual EVR
you're using, although many folks skip the epoch.

  xorg-x11-drv-nouveau-debuginfo.x86_64: W: filename-too-long-for-joliet 
   xorg-x11-drv-nouveau-debuginfo-0.0.10-0.20080221git.fc9.x86_64.rpm
I guess this is unavoidable.

Other minor quibbles: Best to start your release at "1.whatever" instead of
"0.whatever" to distinguish it from the prerelease case.  (Prereleases count
up from 0.1, releases and post-release snapshots count up from 1.)

I don't quite understand the dependency on hwdata, since this package doesn't
install anything into /usr/share/hwdata.

Abbreviated checklist:
* source files match upstream (verified by manual untar/diff)
X doesn't quite meet the versioning guidelines (start post-release snapshots 
   from release 1.x, please)
* specfile is properly named and uses macros consistently.
X The git_version stuff seems somewhat convoluted and seems to not actually 
   work.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
X license text not included in package.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly
* debuginfo package looks complete.
X rpmlint has valid complaints.
? final provides and requires are sane:
   nouveau_drv.so()(64bit)
   xorg-x11-drv-nouveau = 1:0.0.10-0.20080221git.fc9
  =
?  hwdata
   kernel-drm-nouveau = 10
   xorg-x11-server-Xorg >= 1.3.0.0-6


Comment 4 Dave Airlie 2008-02-27 23:17:17 UTC
As I said nouveau has never been released so the 0.whatever is fine.

hwdata depends will be required in the future when we add the pciids, so it
shouldn't be any harm now and I don't want to forget it later.

I've removed the git versioning stuff and fixed the changelog version.

Hopefully it all passes now...

Comment 5 Jason Tibbitts 2008-02-28 20:52:52 UTC
I guess I didn't comminucate my point well enough.  I was just saying that this
package should have a release of "1.20080221git5db7920" instead of
"0.20080221git5db7920".  This package comes from a snapshot made after the
software started calling itself "0.0.10", so it's a post-release snapshot, and
so gets named "X.YYYMMDDwhatever" where X is an integer one or greater (and you
have '0' there).  But really, it's minor.

I'm still seeing the version-release string in the changelog entry not matching
the actual version-release of the package.  But again, that's minor.

I'll approve this because it's pointless to go back and forth over two trivial
one-liners that don't affect the functionality, but please fix up both of them
when you check in.

APPROVED

Comment 6 Dave Airlie 2008-02-28 23:58:56 UTC
New Package CVS Request
=======================
Package Name: xorg-x11-drv-nouveau
Short Description: Xorg X11 nouveau driver for nvidia hardware
Owners: airlied
Branches:
InitialCC: ajax
Cvsextras Commits: yes


Comment 7 Kevin Fenzi 2008-02-29 03:12:40 UTC
cvs done.

Comment 8 Richard Hughes 2008-02-29 19:29:55 UTC
Any way this could be pushed into F9 and the old source removed from
xf-video-nv? Cheers guys.

Comment 9 Peter Lemenkov 2008-05-17 18:02:45 UTC
Reviewed, approved, built and released. So, I'm gonna to close this ticket.


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