This service will be undergoing maintenance at 20:00 UTC, 2017-04-03. It is expected to last about 30 minutes
Bug 225999 - Merge Review: libdrm
Merge Review: libdrm
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Package Reviews List
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:20 EST by Nobody's working on this, feel free to take it
Modified: 2009-02-01 02:37 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-01 02:37:31 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:20:48 EST
Fedora Merge Review: libdrm

http://cvs.fedora.redhat.com/viewcvs/devel/libdrm/
Initial Owner: ajackson@redhat.com
Comment 1 Adam Jackson 2007-02-19 14:45:13 EST
I went ahead and cleaned out the rpmlint warnings.  Only one left is
no-documentation for libdrm-devel, which isn't fixable since there's nothing in
the package that would qualify.
Comment 2 Orcan Ogetbil 2009-01-17 19:56:26 EST
I'll review this package but please update to the latest version first (2.4.4)
Comment 3 Dave Airlie 2009-01-18 06:44:46 EST
already in. don't think we need review anymore
Comment 4 Mamoru TASAKA 2009-01-18 06:53:39 EST
Merge review cannot be closed without properly reviewed.
Comment 5 Orcan Ogetbil 2009-01-19 18:22:55 EST
So, my initial request is still valid. Could you update to the latest version so that we can have a proper review?
Comment 6 Orcan Ogetbil 2009-01-31 00:22:53 EST
So... I don't wanna be rude, but,

Is there something not clear about what I asked?
Comment 7 Dave Airlie 2009-01-31 08:16:55 EST
I've no idea what is going on in this bug. Its a 2 year old review request, that nobody cared about.

Just read the spec file and review it.

You aren't the maintainer so you can't decide when we need to upgrade to the latest upstream version, esp when we are also upstream maintainers, and libdrm is a pretty messy system component to just go upgrading due to links to X/mesa etc.

I'm also nearly sure I did push 2.4.4 into rawhide 2-3 days ago, but again what has the version got to do with the spec file review?

confused.
Dave.
Comment 8 Orcan Ogetbil 2009-01-31 12:05:17 EST
(In reply to comment #7)
> I've no idea what is going on in this bug. Its a 2 year old review request,
> that nobody cared about.
> 
There are plenty of Merge Reviews that are waiting for a reviewer. Sorry, we didn't have the manpower to finish them up in the last two years. People are working on them.

> Just read the spec file and review it.
> 
> You aren't the maintainer so you can't decide when we need to upgrade to the
> latest upstream version, esp when we are also upstream maintainers, and libdrm
> is a pretty messy system component to just go upgrading due to links to X/mesa
> etc.
> 
Thank you for the explanation. Being verbose just *helps*.

> I'm also nearly sure I did push 2.4.4 into rawhide 2-3 days ago, 
Again, I'm sorry. I didn't check the rawhide SPEC. I was busy with monitoring many SPEC files and this one just slipped through. It would be nice if the maintainer wrote about the update here.


> but again what
> has the version got to do with the spec file review?
> 
> confused.
> Dave.

Dave, this is not a "SPEC file review". It is a "package review". The former is only a subset of the latter. For reference, look at 
    http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
Some of these elements cannot be checked within SPEC file only and they need to be re-checked with each upstream version.

I'll do the "package review" now.
Comment 9 Orcan Ogetbil 2009-01-31 19:46:12 EST
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?

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

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

* Now, the rpmlint complaints:
    libdrm.src: W: patch-not-applied Patch2: libdrm-2.4.0-no-freaking-mknod.patch
should be removed if it's useless
    libdrm.src: W: strange-permission make-git-snapshot.sh 0755
we should have 644 for source files
    libdrm.x86_64: W: non-conffile-in-etc /etc/udev/rules.d/91-drm-modeset.rules
this %files entry should be a %config
    libdrm-devel.x86_64: W: no-documentation
at the least, the file "libdrm/ChangeLog" and the "tests" directory can go in here.

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

* libdrm/TODO should go to %doc

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

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

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

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

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

* Parallel make must be supported whenever possible. If it is not supported, this should be noted in the SPEC file as a comment.
Comment 10 Dave Airlie 2009-02-01 02:11:24 EST
(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
Comment 11 Orcan Ogetbil 2009-02-01 02:37:31 EST
You're welcome. I thank you for the nice explanations in a language understandable by non low-level programmers.

Most of the issues I pointed are solved and the remaining ones are defended with valid reasonings.

----------------------------------------------
This Merge Review (libdrm) is APPROVED by oget
----------------------------------------------

Closing the bug.

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