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@xxxxxxxxxx> 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review