[Bug 225999] Merge Review: libdrm

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=225999





--- Comment #10 from Dave Airlie <airlied at redhat.com>  2009-02-01 02:11:24 EDT ---
(In reply to comment #9)
> OK, here's the review, with questions (?), issues (*) and comments (!).
> 
> ? From what you told, I understand that you are the upstream maintainer too. So
> why are the patches? This confuses me. Can't they be integrated into the
> source? Also why use autoreconf?

We ship stuff in Fedora this isn't released fully suitable for upstream yet,
libdrm is a small component in a the big kernel/X stack, nothing can hit a
released libdrm until corresponding code is shipped in the upstream kernel, so
we carry kernel patches + libdrm patches until the code is all in the correct
upstream places.

We use autoreconf because we change Makefile.am's and we need to reconfigure.

> ? Why are those header files are getting removed? And if they are irrelevant,
> why are being installed by the Makefile? An explanation please, preferably in
> the SPEC file as a comment.

Upstream and kernel are still working out ownership of certain header files,
the kernel now installs some header files and we are working on transitioning
libdrm away from this task, its not an overnight task. 2.4.5 will hopefully
have a configure option.

> 
> * Generally, all the patches need to be explained as comments in the SPEC file
> (and they need to be sent upstream but we skip this part). It's best to keep
> the SPEC file at a state where a new package maintainer can take it over easily
> without spending hours to figure out what's going on.

I've put some info in there but no new package maintainer will ever take it
over, libdrm is part of the X stack, we have a team in RH looking after it, if
someone new joins the team they already know what we are up to with libdrm.

> * Now, the rpmlint complaints:
>     libdrm.src: W: patch-not-applied Patch2:
> libdrm-2.4.0-no-freaking-mknod.patch

Okay I can drop that I think we fixed it upstream.

> should be removed if it's useless
>     libdrm.src: W: strange-permission make-git-snapshot.sh 0755
> we should have 644 for source files

This isn't a source file its a script you run to make a snapshot of libdrm from
git for shipping it never gets shipped. so I'm leaving it as-is.

>     libdrm.x86_64: W: non-conffile-in-etc
> /etc/udev/rules.d/91-drm-modeset.rules
> this %files entry should be a %config

good point.

>     libdrm-devel.x86_64: W: no-documentation
> at the least, the file "libdrm/ChangeLog" and the "tests" directory can go in
> here.

the changelog is no longer generated, the tests aren't actually tests, they are
like little apps, that aren't really used by anyone anymore, so I don't want to
ship or even try and support them.

> 
> * %description should be descriptive, not a carbon copy of the summary.

Not sure we can say more, that's all it is, its the runtime library supporting
the direct rendering management infrastructure. Nobody uses this library
outside of mesa and X stuff, its not general purpose by any means. it used to
be part of the X server and mesa internals.

> 
> * libdrm/TODO should go to %doc

Sorely out of date so no point.

> 
> * The upstream should be advised to put a copy of the full text of the license
> in a seperate COPYING file.

its MIT/BSD if a patch appeared upstream I'd apply it but I've a lot bigger
things to worry about, but its on every file in the package.

> 
> ! The timestamp of the sourcefile is wrong (should be downloaded with "wget -N"
> or such)

will fix that next revision upload hopefully.

> 
> ! BR: pkgconfig is not required since libxcb-devel will pull that up.

I'd prefer to keep it explicit just in case we figure out we don't need
libxcb-devel later.

> 
> * /etc/udev/rules.d is not owned. So we must require the package that owns it
> (which, I think, is udev).

done.

> 
> * Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for
> directory ownership and usability). This applies to the devel package.

done.

> 
> * Parallel make must be supported whenever possible. If it is not supported,
> this should be noted in the SPEC file as a comment.

it should be supported not sure why it was disabled - re-enabled

Thanks for the review

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.