Bug 220890 - Review Request: libcdaudio - Control operation of a CD-ROM when playing audio CDs
 Summary: Review Request: libcdaudio - Control operation of a CD-ROM when playing audio...
 Status: Product: CLOSED CURRENTRELEASE Aliases: None Fedora Fedora Package Review (Show other bugs) --- rawhide All Linux medium Severity medium --- Target Release: --- Jarod Wilson Fedora Package Reviews List FE-ACCEPT Show dependency tree / graph

 Reported: 2006-12-28 08:00 EST by Axel Thimm 2008-08-02 19:40 EDT (History) 0 users 0.99.12p2-8 Bug Fix --- 2007-03-13 10:48:25 EDT --- --- --- --- --- --- --- jarod: fedora‑review+ dennis: fedora‑cvs+

 Axel Thimm 2006-12-28 08:00:34 EST Spec URL: http://dl.atrpms.net/all/libcdaudio.spec SRPM URL: http://dl.atrpms.net/all/libcdaudio-0.99.12p2-7.at.src.rpm Description: libcdaudio is a library designed to provide functions to control operation of a CD-ROM when playing audio CDs. It also contains functions for CDDB and CD Index lookup. Parag AN(पराग) 2006-12-28 09:50:10 EST rpmlint on SRPM gave me W: libcdaudio invalid-license GPL2  Michael Schwendt 2006-12-29 12:20:50 EST * /usr/bin/libcdaudio-config needs a patch for option --libs, because it contains a hardcoded \${exec_prefix}/lib path. * RPM Group better is changed to: System Environment/Libraries Axel Thimm 2006-12-29 13:05:03 EST God catch, thanks! Axel Thimm 2007-01-01 09:33:04 EST Spec URL: http://dl.atrpms.net/all/libcdaudio.spec SRPM URL: http://dl.atrpms.net/all/libcdaudio-0.99.12p2-8.at.src.rpm * Fri Dec 29 2006 Axel Thimm - 0.99.12p2-8 - Change Group tag. - Fix libcdaudio-config for libdir != %%{_prefix}/lib.  Jarod Wilson 2007-02-19 15:19:30 EST * source files match upstream: - 15de3830b751818a54a42899bd3ae72c libcdaudio-0.99.12p2.tar.gz - 15de3830b751818a54a42899bd3ae72c libcdaudio-0.99.12p2-orig.tar.gz * package meets naming and versioning guidelines * specfile is properly named, is cleanly written and uses macros consistently. * dist tag is present. ! BuildRoot is not correct, should be: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) * license field matches the actual license. * license is open source-compatible. GPL2, License text included in package. - rpmlint complains about GPL2, but rpmlint probably shouldn't... * latest version is being packaged. ! BuildRequires are proper. - Technically, nothing wrong with gcc-c++, but its listed in the packaging guidelines as an exception that should be left out, since its always in the minimum build root. * compiler flags are appropriate. * %clean is present. * package builds in mock (F7/x86_64). * package installs properly * debuginfo package looks complete. * rpmlint is silent. - Only complaint is the Warning about GPL2, which I say we ignore. * final provides and requires are sane: libcdaudio provides/requires: -- libcdaudio.so.1()(64bit) libcdaudio = 0.99.12p2-8.fc7 -- libcdaudio.so.1()(64bit) libcdaudio-devel provides/requires: -- libcdaudio-devel = 0.99.12p2-8.fc7 -- libcdaudio = 0.99.12p2-8.fc7 libcdaudio.so.1()(64bit) pkgconfig * %check is n/a * no shared libraries are added to the regular linker search paths. * owns the directories it creates. * doesn't own any directories it shouldn't. * no duplicates in %files. * file permissions are appropriate. * no scriptlets present. * code, not content. * documentation is small, so no -docs subpackage is necessary. * %docs are not necessary for the proper functioning of the package. * headers in -devel - cdaudio.h ought to be installed w/-p to preserve timestamps, but that looks to be upstream's fault * pkgconfig files in -devel, properly requires pkgconfig * no libtool .la droppings. * not a GUI app. I think the only MUSTFIX item is the BuildRoot, but I'd suggest also dropping the BR: on gcc-c++. Whether or not to patch the Makefile to install the header w/-p I'll leave up to you. Jarod Wilson 2007-02-19 15:28:04 EST Minor correction after talking with some folks on the license issue: it should be reduced to simply "GPL", since the included readme doesn't list an explicit version of the GPL. -- libcdaudio is distributed under the GNU Library General Public License, included in this package under the top level source directory in the file COPYING. -- Axel Thimm 2007-02-20 05:24:09 EST Thanks for the review! On buildroots we recently voted against that buildroot and currently I'm trying to get the mandatory insanity of the guidelines, so let's keep it open for now, I hope for the best ;) On the license: There is a COPYING file that explicitely states > GNU GENERAL PUBLIC LICENSE > Version 2, June 1991 And the README says > libcdaudio is distributed under the GNU Library General Public > License, included in this package under the top level source directory > in the file COPYING. Jarod Wilson 2007-02-20 10:32:26 EST (In reply to comment #7) > Thanks for the review! No problem, sorry it sat so long w/o any attention. > On buildroots we recently voted against that buildroot and currently I'm trying > to get the mandatory insanity of the guidelines, so let's keep it open for now, > I hope for the best ;) Hm... Talked to some FESCo folks, they say that the buildroot specified in the guidelines was accepted by FESCo via a vote at FUDCon as the standard for now, but with work that needs doing at the rpm level to properly address concerns. So for the moment, I'm told that it does have to be as specified... Who voted against it? > On the license: There is a COPYING file that explicitely states > > GNU GENERAL PUBLIC LICENSE > > Version 2, June 1991 > > And the README says > > > libcdaudio is distributed under the GNU Library General Public > > License, included in this package under the top level source directory > > in the file COPYING. I saw that the actual license file says v2, but that's the case with almost every GPL-licensed bits these days. My understanding from the FESCo folks I was talking to is that unless the author says v2 is explicitly required, to just keep the license field general, specifying just "GPL". Personally, I don't really care either way, just trying to follow the letter of the law, so to speak. (Though I like "GPLv2" better than "GPL2".) Axel Thimm 2007-02-20 15:03:16 EST The buildroot was voted on in the FPC and later ratified by fesco. Since the vote in the FPC was under peculiar circumstances the issue was raised again the next week and the buildroot changed (by vote) to one w/o id -un. Currently I'm trying to persuade the FPC to drop the mandatory part on buildroots and simply allow any sane buildroot. Jarod Wilson 2007-02-20 16:34:04 EST I'd like for someone from the FPC and/or FESCo to comment. People I'm finding on IRC say it was agreed it should be changed to something without the id, call, instead using a mktemp call, but that had unexpected negative side-effects, so everything is currently still as it has been. So at the moment, the guidelines still say that the mandatory buildroot is the one previously quoted. Of course, I agree that anything sane should be fine. Above and beyond that, I really don't think we should have to care what the buildroot is at all. If its supposed to be the same in all specs, it should be specified by the build system, not duplicated in every spec. Axel Thimm 2007-02-20 17:02:40 EST > I'd like for someone from the FPC and/or FESCo to comment. I'm from FPC. Yes, we went for using mktemp -ud as the lesser evil. The expected side-effects are that stepped rpmbuilds (using --short-circuit for example) won't work anymore unless you supply a buildroot on the command line. > Of course, I agree that anything sane should be fine. Above and beyond that, I > really don't think we should have to care what the buildroot is at all. If its > supposed to be the same in all specs, it should be specified by the build > system, not duplicated in every spec. +\infty  Jarod Wilson 2007-02-20 17:22:34 EST (In reply to comment #11) > > I'd like for someone from the FPC and/or FESCo to comment. > > I'm from FPC. Ah, I was completely unaware of that. :) It seems there has been a lack of communication between FPC and FESCo. At the very least, some members of FESCo appear to be unaware of any changes made to the packaging guidelines... > Yes, we went for using mktemp -ud as the lesser evil. The expected > side-effects are that stepped rpmbuilds (using --short-circuit for example) > won't work anymore unless you supply a buildroot on the command line. So where does this leave us with respect to what's currently spelled out as being mandatory in the guidelines? > > Of course, I agree that anything sane should be fine. Above and beyond that, I > > really don't think we should have to care what the buildroot is at all. If its > > supposed to be the same in all specs, it should be specified by the build > > system, not duplicated in every spec. > > +\infty :) So how about just putting in the current "Mandatory" BuildRoot value as listed in the current packaging guidelines for now, and once the dust settles for The Grand Future of BuildRoot, change it to whatever the new-and-improved version is? Dennis Gilmore 2007-02-20 17:31:13 EST http://fedoraproject.org/wiki/Packaging/Guidelines#head-f196e7b2477c2f5dd97ef64e8eacddfb517f1aa1 says The MANDATORY value for the BuildRoot tag is %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) its mandatory use it regardless of your personal beliefs the community that you are part of agreed to it. if it changes at some point in the future you will be free to change it then. until then please abide by the guidelines. Ville Skyttä 2007-02-20 17:45:03 EST (In reply to comment #11) > I'm from FPC. Yes, we went for using mktemp -ud as the lesser evil. The expected > side-effects are that stepped rpmbuilds (using --short-circuit for example) > won't work anymore unless you supply a buildroot on the command line. Could you post a reproducer for this to fedora-packaging? -bi --short-circuit builds work just fine for me with a mktemp -ud BuildRoot when it's specified in a specfile. OTOH it doesn't work for whatever reason if I set it in ~/.rpmmacros (gets evaluated twice in that case), but perhaps that's what you meant? Axel Thimm 2007-02-20 19:38:07 EST (In reply to comment #12) > (In reply to comment #11) > It seems there has been a lack of communication between FPC and FESCo. At the > very least, some members of FESCo appear to be unaware of any changes made to > the packaging guidelines... Well, fesco and fpc overlap by > 50% or more, so at least that amount of fesco should be aware of fpc topics automatically and have perfect communication. :) (In reply to comment #13) > its mandatory use it regardless of your personal beliefs the community that > you are part of agreed to it. if it changes at some point in the future Ahem, the "future" was a week ago. FPC reevaluated this decision and dropped that buildroot in favour of the mktemp one, the voting was unanimously. But I don't know whether it was persented to fesco on Thursday and if, whether it was ratified. > you will be free to change it then. until then please abide by the > guidelines. Please check fedora-packaging (In reply to comment #14) > (In reply to comment #11) > > I'm from FPC. Yes, we went for using mktemp -ud as the lesser evil. The expected > > side-effects are that stepped rpmbuilds (using --short-circuit for example) > > won't work anymore unless you supply a buildroot on the command line. > > Could you post a reproducer for this to fedora-packaging? -bi --short-circuit > builds work just fine for me with a mktemp -ud BuildRoot when it's specified in > a specfile. OTOH it doesn't work for whatever reason if I set it in > ~/.rpmmacros (gets evaluated twice in that case), but perhaps that's what you meant? Sorry, Ville, I think you are 100% correct and this is a Red Herring. I was just quoting racor on this w/o thinking too much on my own. Indeed, since it's not quite sane to use the buildroot outside of %install and %clean, all that is slightly broken is separating building and cleaning, but IMHO we can live with that. Of course you will find some people that claim that %buildroot should be available during %build, too, and I remember such a discussion on fedora-packaging, perhaps racor was even one of them. We should better create a guideline against using %buildroot in %prep and %build. :) So even less evil with the mktemp variant.  Michael Schwendt 2007-02-21 16:27:31 EST > -bi --short-circuit builds work just fine for me with a > mktemp -ud BuildRoot when it's specified in a specfile. Define "work just fine". You do get a new buildroot with every invocation of rpmbuild, don't you? Effectively, these newly created buildroots pile up to a gigantic pile of crap. Subsequent invocations of rpmbuild do not remove the old buildroots automatically as it is done with a predictable buildroot filename. In situations when --short-circuit is really helpful, the pile of buildroots gets inconvenient to navigate in and requires work-arounds. Hence a mandatory buildroot that uses mktemp would turn against packagers and would be a bad decision.  Axel Thimm 2007-02-21 16:37:25 EST (In reply to comment #16) > You do get a new buildroot with every invocation of rpmbuild, don't you? No, only on rpmbuilds that go into %install. That's due to using the -u switch of mktemp. Quite clever and credits go to Ville for that. > In situations when --short-circuit is really helpful, the pile of buildroots > gets inconvenient Forget I mentioned --short-circuit, Ville's trick deals fine with that, and I simply trusted some mails I had read w/o checking, so --short-circuit is not an issue. The only remaining bit is running %install w/o %clean. This will indeed leave buildroots behind.  Michael Schwendt 2007-02-21 16:52:51 EST I did refer to --short-circuit commands in my entire comment. Please do try the proposed buildroot with --short-circuit and prove that it doesn't do what I've written in comment 16. Option -u does not prevent mktemp from returning a new path with every invocation. > The only remaining bit is running %install w/o %clean. ?? But --short-circuit -i does execute %install, so that is where the crap starts.  Axel Thimm 2007-02-21 17:27:16 EST (In reply to comment #16) > You do get a new buildroot with every invocation of rpmbuild, don't you? > Effectively, these newly created buildroots pile up to a gigantic pile of > crap. (In reply to comment #18) > Please do try the proposed buildroot with --short-circuit and > prove that it doesn't do what I've written in comment 16. Option > -u does not prevent mktemp from returning a new path with > every invocation. Prove? It's true that it *returns* a new buildroot on every invocation, but it does not *create* it, no "gigantic pile of crap" is generated by merely invoking rpmbuild as you wrote. E.g. rpmbuild -bp/-bc are not creating anything. And that's why --short-circuit has nothing to do with this here, unless you would had been suggesting packages that use %buildroot in %prep and %build, which (almost) all agree to be A Bad Thing. Anyway whoever is smart enough to make diligent use of --short-circuit will be able to use his favourite external %buildroot setting as well. But for the normal non-short-circuit case there is still a small bitter pill left, that seperated %install and %clean of rpmbuilds will leave buildroots behind. Ceterum censeo clausulum definendam buildrootium coactum esse esse delendam. Ceterum censeo  Michael Schwendt 2007-02-21 17:41:19 EST I still only refer to --short-circuit commands. Get back to the quote that started this: > -bi --short-circuit builds work just fine for me with a > mktemp -ud BuildRoot when it's specified in a specfile. Do you see? -bi --short-circuit > Anyway whoever is smart enough to make diligent use of > --short-circuit will be able to use his favourite external > %buildroot setting as well. So, I must specify a BuildRoot in the spec only to override it in ~/.rpmmacros because it is over-engineered crap? Sad. Ville Skyttä 2007-02-22 02:43:09 EST Please take this discussion to the fedora-packaging mailing list. Axel Thimm 2007-03-06 13:35:31 EST Last week the buildroot proposal http://fedoraproject.org/wiki/Packaging/Guidelines?action=recall&rev=92#head-b4fdd45fa76cbf54c885ef0836361319ab962473 was ratified by both fpc and fesco and I just edited the guidelines. :)  Jarod Wilson 2007-03-06 13:51:44 EST Yep, saw that and was meaning to dig this bug back up and throw an APPROVED stamp on it accordingly. Package APPROVED, import at will. Jarod Wilson 2007-03-06 13:53:52 EST Whoops, wrong blocker bug... Will set the review + flag too... Axel Thimm 2007-03-06 14:16:28 EST Thanks! New Package CVS Request ======================= Package Name: libcdaudio Short Description: Control operation of a CD-ROM when playing audio CDs Owners: Axel.Thimm@ATrpms.net Branches: FC-5 FC-6 InitialCC:  Dennis Gilmore 2007-03-07 14:08:25 EST branched Jarod Wilson 2007-03-13 10:33:59 EDT ...and built. Can this bug be closed now? Axel Thimm 2007-03-13 10:48:25 EDT Thanks again for the review!