Fedora Merge Review: libdrm http://cvs.fedora.redhat.com/viewcvs/devel/libdrm/ Initial Owner: ajackson
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.
I'll review this package but please update to the latest version first (2.4.4)
already in. don't think we need review anymore
Merge review cannot be closed without properly reviewed.
So, my initial request is still valid. Could you update to the latest version so that we can have a proper review?
So... I don't wanna be rude, but, Is there something not clear about what I asked?
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.
(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.
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.
(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
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.