Spec URL: http://people.redhat.com/~jbrier/rpms/xwax.spec SRPM URL: http://people.redhat.com/~jbrier/rpms/xwax-0.9-2.fc14.src.rpm Description: xwax is open-source vinyl emulation software for Linux. It allows DJs and turntablists to playback digital audio files (MP3, Ogg Vorbis, FLAC, AAC and more), controlled using a normal pair of turntables via timecoded vinyls. It's designed for both beat mixing and scratch mixing. Needle drops, pitch changes, scratching, spinbacks and rewinds are all supported, and feel just like the audio is pressed onto the vinyl itself. The focus is on an accurate vinyl feel which is efficient, stable and fast. --- This is my first package and I need a sponsor. Note this package can be enhanced by mpg123 and it could be considered "not useful without external bits." I talked to FESCo about this and got them to allow it: https://fedorahosted.org/fesco/ticket/583
NB: I can't sponsor you, this is just an informal review. Have just run this through Tim Lauridsen's review tool, and it's found some problems: Package Review ============== Key: - = N/A x = Check ! = Problem ? = Not evaluated [x] : MUST - Package successfully compiles and builds into binary rpms on at least one supported architecture. [x] : MUST - Package does not contain any libtool archives (.la) [x] : MUST - Package use %makeinstall only when make install DESTDIR=... doesn't work. [x] : MUST - Package is named according to the Package Naming Guidelines. [x] : MUST - Sources used to build the package matches the upstream source, as provided in the spec URL. MD5SUM this package : 37152a16cbeb6878818ca406959af9b2 MD5SUM upstream package : 37152a16cbeb6878818ca406959af9b2 [x] : MUST - Spec file name must match the spec package %{name}, in the format %{name}.spec. [-] : MUST - %config files are marked noreplace or the reason is justified. [-] : MUST - Package contains a properly installed %{name}.desktop using desktop-file-install file if it is a GUI application. [-] : MUST - Fully versioned dependency in subpackages, if present. [-] : MUST - Header files in -devel subpackage, if present. [-] : MUST - ldconfig called in %post and %postun if required. [-] : MUST - License file installed when any subpackage combination is installed. [-] : MUST - The spec file handles locales properly. [-] : MUST - No %config files under /usr. [-] : MUST - Development .so files in -devel subpackage, if present. [-] : MUST - Static libraries in -static subpackage, if present. [!] : MUST - Spec file lacks Packager, Vendor, PreReq tags. Found : Packager: Adam Huffman <bloch> [!] : MUST - Rpmlint output is silent. rpmlint xwax-0.9-2.fc16.i686.rpm ================================================================================ xwax.i686: W: spelling-error %description -l en_US turntablists -> turntables, turntable, establishments xwax.i686: W: spelling-error %description -l en_US timecoded -> time coded, time-coded, timecard xwax.i686: W: spelling-error %description -l en_US spinbacks -> spin backs, spin-backs, finbacks xwax.i686: W: invalid-license GPL 1 packages and 0 specfiles checked; 0 errors, 4 warnings. ================================================================================ rpmlint xwax-debuginfo-0.9-2.fc16.i686.rpm ================================================================================ xwax-debuginfo.i686: W: invalid-license GPL xwax-debuginfo.i686: E: debuginfo-without-sources 1 packages and 0 specfiles checked; 1 errors, 1 warnings. ================================================================================ rpmlint xwax-0.9-2.fc16.src.rpm ================================================================================ xwax.src: W: spelling-error %description -l en_US turntablists -> turntables, turntable, establishments xwax.src: W: spelling-error %description -l en_US timecoded -> time coded, time-coded, timecard xwax.src: W: spelling-error %description -l en_US spinbacks -> spin backs, spin-backs, finbacks xwax.src: W: invalid-license GPL xwax.src:12: W: macro-in-comment %40redhat 1 packages and 0 specfiles checked; 0 errors, 5 warnings. ================================================================================ [ ] : MUST - Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [ ] : MUST - %build honors applicable compiler flags or justifies otherwise. [ ] : MUST - All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [ ] : MUST - Package contains no bundled libraries. [ ] : MUST - Changelog in prescribed format. [ ] : MUST - Sources contain only permissible code or content. [ ] : MUST - Macros in Summary, %description expandable at SRPM build time. [ ] : MUST - Package requires other packages for directories it uses. [ ] : MUST - Package uses nothing in %doc for runtime. [ ] : MUST - Package is not known to require ExcludeArch. [ ] : MUST - Permissions on files are set properly. [ ] : MUST - Package does not contain duplicates in %files. [ ] : MUST - Large documentation files are in a -doc subpackage, if required. [ ] : MUST - If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [ ] : MUST - License field in the package spec file matches the actual license. [ ] : MUST - Package consistently uses macros. instead of hard-coded directory names. [ ] : MUST - Package meets the Packaging Guidelines. [ ] : MUST - Package does not generates any conflict. [ ] : MUST - Package does not contains kernel modules. [ ] : MUST - Package contains no static executables. [ ] : MUST - Package obeys FHS, except libexecdir and /usr/target. [ ] : MUST - Package must own all directories that it creates. [ ] : MUST - Package does not own files or directories owned by other packages. [ ] : MUST - Package installs properly. [ ] : MUST - Rpath absent or only used for internal libs. [ ] : MUST - Package is not relocatable. [ ] : MUST - Requires correct, justified where necessary. [ ] : MUST - Spec file is legible and written in American English. [ ] : MUST - Package contains a SysV-style init script if in need of one. [ ] : MUST - File names are valid UTF-8. [ ] : MUST - Useful -debuginfo package or justification otherwise. [x] : SHOULD - Reviewer should test that the package builds in mock. [x] : SHOULD - Dist tag is present. [x] : SHOULD - SourceX is a working URL. [x] : SHOULD - Spec use %global instead of %define. [-] : SHOULD - Uses parallel make. [-] : SHOULD - The placement of pkgconfig(.pc) files are correct. [!] : SHOULD - SourceX / PatchY prefixed with %{name}. Patch0: xwax-import.patch (xwax-import.patch) [ ] : SHOULD - If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [ ] : SHOULD - No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [ ] : SHOULD - Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [ ] : SHOULD - Package functions as described. [ ] : SHOULD - Latest version is packaged. [ ] : SHOULD - Package does not include license text files separate from upstream. [ ] : SHOULD - Man pages included for all executables. [ ] : SHOULD - Patches link to upstream bugs/comments/lists or are otherwise justified. [ ] : SHOULD - Scriptlets must be sane, if used. [ ] : SHOULD - Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [ ] : SHOULD - Package should compile and build into binary rpms on all supported architectures. [ ] : SHOULD - %check is present and all tests pass. [ ] : SHOULD - Packages should try to preserve timestamps of original installed files. Issues: [!] : MUST - Spec file lacks Packager, Vendor, PreReq tags. Found : Packager: Adam Huffman <bloch> [!] : MUST - Rpmlint output is silent. rpmlint xwax-0.9-2.fc16.i686.rpm ================================================================================ xwax.i686: W: spelling-error %description -l en_US turntablists -> turntables, turntable, establishments xwax.i686: W: spelling-error %description -l en_US timecoded -> time coded, time-coded, timecard xwax.i686: W: spelling-error %description -l en_US spinbacks -> spin backs, spin-backs, finbacks xwax.i686: W: invalid-license GPL 1 packages and 0 specfiles checked; 0 errors, 4 warnings. ================================================================================ rpmlint xwax-debuginfo-0.9-2.fc16.i686.rpm ================================================================================ xwax-debuginfo.i686: W: invalid-license GPL xwax-debuginfo.i686: E: debuginfo-without-sources 1 packages and 0 specfiles checked; 1 errors, 1 warnings. ================================================================================ rpmlint xwax-0.9-2.fc16.src.rpm ================================================================================ xwax.src: W: spelling-error %description -l en_US turntablists -> turntables, turntable, establishments xwax.src: W: spelling-error %description -l en_US timecoded -> time coded, time-coded, timecard xwax.src: W: spelling-error %description -l en_US spinbacks -> spin backs, spin-backs, finbacks xwax.src: W: invalid-license GPL xwax.src:12: W: macro-in-comment %40redhat 1 packages and 0 specfiles checked; 0 errors, 5 warnings. ================================================================================ You can ignore the spelling complaints, but you need to fix the licensing and investigate the debuginfo problem.
You should also consider using macros consistently i.e. %{buildroot} instead of $RPM_BUILD_ROOT
Spec URL: http://people.redhat.com/~jbrier/rpms/xwax.spec SRPM URL: http://people.redhat.com/~jbrier/rpms/xwax-0.9-3.fc14.src.rpm (In reply to comment #1) > NB: I can't sponsor you, this is just an informal review. Thank you very much for this! > You can ignore the spelling complaints, but you need to fix the licensing and > investigate the debuginfo problem. I believe I have fixed both issues.
Thanks for the new version. Some more comments: - I assume you've added the Requires: line because of the issue mentioned regarding decoders? Normally the automatic checker is sufficient. - you need to use macros in the %files section e.g %{_bindir}/xwax %{_libexecdir}/xwax/xwax-import and you should own the %{_libexecdir}/xwax/ directory - you don't need the full path for those doc files %doc CHANGES COPYING README should be enough and you should use macros for the manpage %doc %{_mandir}/man1/xwax.1.gz To help persuade someone to sponsor you, it would be a good idea to submit a couple more new packages and do some informal reviews yourself.
(In reply to comment #4) > Thanks for the new version. Some more comments: > > - I assume you've added the Requires: line because of the issue mentioned > regarding decoders? Normally the automatic checker is sufficient. Yes, the decoder is only referenced in a bash script so I don't think the automatic checker would catch it. > - you need to use macros in the %files section e.g > > %{_bindir}/xwax > %{_libexecdir}/xwax/xwax-import > > and you should own the %{_libexecdir}/xwax/ directory > > - you don't need the full path for those doc files > > %doc CHANGES COPYING README > > should be enough and you should use macros for the manpage > > %doc %{_mandir}/man1/xwax.1.gz > > > To help persuade someone to sponsor you, it would be a good idea to submit a > couple more new packages and do some informal reviews yourself. I will look into all of the above. Thanks again.
Did anything ever happen here? It's been well over a year now.
(In reply to comment #6) > Did anything ever happen here? It's been well over a year now. No. I have just been lazy. That said I did upgrade my spec for the 1.2 version if you don't mind building it yourself: http://people.redhat.com/~jbrier/rpms/xwax.spec I have an x86_64 rpm if that's your arch too: http://people.redhat.com/~jbrier/rpms/xwax-1.2-1.fc17.x86_64.rpm ...but I haven't looked into fixing the issues mentioned in comment 5 as I said because I've been lazy. I also understand that even if you have a perfect spec that the new package may not be accepted until you have demonstrated activity and understanding of Fedora packaging by reviewing other requests and such. I just didn't make that effort :-( However I do have a bit of a renewed interest in this currently so maybe I will jump on it. Thanks for the push.
Unfortunately it's not possible to build from just a spec since you need the patch. Do note that sponsors differ in what they'll want to see from those they sponsor. Really what we're trying to gauge is whether you want to participate in the community and aren't just planning to dump some package on the distribution that you don't plan to actually maintain. Of course, it's impossible to predict the future so some sponsors want to actually see community participation. Personally I think that anyone who waits around for a year with no progress and is still interested has demonstrated all I need to see.
(In reply to comment #8) > Unfortunately it's not possible to build from just a spec since you need the > patch. I commented that out just cause I wanted the new xwax 1.2 quick and dirty on my laptop ;-) #%patch0 -p1 -b .import > Do note that sponsors differ in what they'll want to see from those they > sponsor. Really what we're trying to gauge is whether you want to > participate in the community and aren't just planning to dump some package > on the distribution that you don't plan to actually maintain. Of course, > it's impossible to predict the future so some sponsors want to actually see > community participation. Personally I think that anyone who waits around > for a year with no progress and is still interested has demonstrated all I > need to see. Ah, that is good to know. I will look into this again.
SPEC file updated: http://people.redhat.com/~jbrier/rpms/xwax.spec patch if you don't want to get it from the SRPM: http://people.redhat.com/~jbrier/rpms/xwax-import.patch SRPM: http://people.redhat.com/~jbrier/rpms/xwax-1.2-2.fc17.src.rpm (In reply to comment #4) > Thanks for the new version. Some more comments: > > - I assume you've added the Requires: line because of the issue mentioned > regarding decoders? Normally the automatic checker is sufficient. > > - you need to use macros in the %files section e.g > > %{_bindir}/xwax > %{_libexecdir}/xwax/xwax-import Those worked as you suggested > and you should own the %{_libexecdir}/xwax/ directory Not sure what you mean by this. I don't see any special ownership provisions explained here: https://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir Currently it looks like this: drwxr-xr-x. 2 root root 4096 Jun 10 15:14 xwax > - you don't need the full path for those doc files > > %doc CHANGES COPYING README This is problematic because I end up with installed but unpackaged files (/usr/share/doc/xwax/$files). I assume they are unpackaged because '%doc CHANGES COPYING README w/o the full path wants to put them in /usr/share/doc/%{name}-%{version} but the Makefile uses just the /usr/share/doc/xwax path. I can fix that by passing DOCDIR=%{buildroot}/%_defaultdocdir/%{name}-%{version} to the 'make install' command in the %install section but, I end up with an rpm that contains the following duplicate doc files: [rpmbuild@aphrodite rpmbuild]$ rpm -qlp RPMS/x86_64/xwax-1.2-2.fc17.x86_64.rpm /usr/bin/xwax /usr/libexec/xwax/xwax-import /usr/libexec/xwax/xwax-scan /usr/share/doc/xwax-1.2 /usr/share/doc/xwax-1.2/CHANGES /usr/share/doc/xwax-1.2/COPYING /usr/share/doc/xwax-1.2/README /usr/share/doc/xwax-1.2/xwax /usr/share/doc/xwax-1.2/xwax/CHANGES /usr/share/doc/xwax-1.2/xwax/COPYING /usr/share/doc/xwax-1.2/xwax/README /usr/share/man/man1/xwax.1.gz The Makefile is at fault I think: $(INSTALL) -m 0644 CHANGES $(DOCDIR)/xwax/CHANGES $(INSTALL) -m 0644 COPYING $(DOCDIR)/xwax/COPYING $(INSTALL) -m 0644 README $(DOCDIR)/xwax/README It always uses $(DOCDIR)/xwax so there is no way to get rid of that directory w/o patching the Makefile, right? > should be enough and you should use macros for the manpage > > %doc %{_mandir}/man1/xwax.1.gz That works
OK, let's get this done. Note that everything I write here is valid for Fedora, and I'm pretty sure for RHEL6 as well. RHEL5 has different requirements and I'm not considering those in this review. First I'll address the two questions above. Indeed, this package doesn't own /usr/libexec/xwax, and it needs to do so. You should just remove the two relevant lines from %files and add %{_libexecdir}/xwax/ The relevant guideline is http://fedoraproject.org/wiki/Packaging:UnownedDirectories There are at least three ways to deal with the duplicated documentation issue. You could patch the makefile. You could let the makefile install what it wants and then just delete the stuff you don't want. Or, you could use this horrible trick: make ALSA=yes JACK=yes install PREFIX=%{buildroot}/%{_prefix} EXECDIR=%{buildroot}/%{_libexecdir}/%{name} DOCDIR=/tmp which works great for me. It just installs those files in some place that doesn't matter but which is outside the buildroot, so it won't end up in the final package. On to the review.... Builds fine on x86_64 rawhide. rpmlint has a couple of complaints: xwax.src:12: W: macro-in-comment %40redhat xwax.src:53: W: macro-in-comment %doc You need to be very careful about trying to "comment out" rpm macros since '#' isn't actually a comment character in a spec file. Macros are still expanded, and if the macro happens to be multi-line then all sorts of bad things can happen. The "for Linux" bit in your summary and description does seem kind of funny, as this is a Fedora package so by definition it's for Linux. Not a big deal, though. BuildRoot: is unnecessary in Fedora, as is the "rm -rf" at the start of %install, the entire %clean section and the %defattr line. You can remove all of them. You don't technically need to have a build dependency on SDL-devel since SDL_ttf-devel requires it, but this doesn't really matter. Thanks for the rationale for not including a .desktop file. I can understand how it wouldn't really work well, and how a menu item for this wouldn't be useful. So really this only needs a few quick fixes. * source files match upstream. sha256sum: 2e35aafb2a08d29c611e19a54b3e45b5c7243cf53b2aec1634c87f01fb35e6a9 xwax-1.2.tar.gz * package meets naming and versioning guidelines. * specfile is properly named, is cleanly written and uses macros consistently. * summary is OK. * description is OK. * dist tag is present. * license field matches the actual license. * license is open source-compatible. * license text included in package. * latest version is being packaged. * BuildRequires are proper. * compiler flags are appropriate. * package builds in mock (rawhide, x86_64). * package installs properly. * debuginfo package looks complete. X rpmlint has a valid complaint * final provides and requires are sane: xwax = 1.2-2.fc18 xwax(x86-64) = 1.2-2.fc18 = /bin/sh cdparanoia libSDL-1.2.so.0()(64bit) libSDL_ttf-2.0.so.0()(64bit) libasound.so.2()(64bit) libasound.so.2(ALSA_0.9)(64bit) libasound.so.2(ALSA_0.9.0rc4)(64bit) libjack.so.0()(64bit) sox * no bundled libraries. X fails to own /usr/libexec/xwax * doesn't own any directories it shouldn't. X documentation is duplicated. * file permissions are appropriate. * no generically named files. * code, not content. * documentation is small, so no -doc subpackage is necessary. * %docs are not necessary for the proper functioning of the package. * no libtool .la files.
(In reply to comment #11) > OK, let's get this done. Note that everything I write here is valid for > Fedora, and I'm pretty sure for RHEL6 as well. RHEL5 has different > requirements and I'm not considering those in this review. Thanks for the review. SPEC updated per your suggestions: http://people.redhat.com/~jbrier/rpms/xwax.spec SRPM updated per your suggestions: http://people.redhat.com/~jbrier/rpms/xwax-1.2-3.fc17.src.rpm > First I'll address the two questions above. > > Indeed, this package doesn't own /usr/libexec/xwax, and it needs to do so. > You should just remove the two relevant lines from %files and add > %{_libexecdir}/xwax/ > The relevant guideline is > http://fedoraproject.org/wiki/Packaging:UnownedDirectories > > There are at least three ways to deal with the duplicated documentation > issue. > You could patch the makefile. You could let the makefile install what it > wants and then just delete the stuff you don't want. Or, you could use this > horrible trick: > make ALSA=yes JACK=yes install PREFIX=%{buildroot}/%{_prefix} > EXECDIR=%{buildroot}/%{_libexecdir}/%{name} DOCDIR=/tmp > which works great for me. It just installs those files in some place that > doesn't matter but which is outside the buildroot, so it won't end up in the > final package. Hah! Nice trick. I'll take it. > On to the review.... Builds fine on x86_64 rawhide. rpmlint has a couple of > complaints: > > xwax.src:12: W: macro-in-comment %40redhat > xwax.src:53: W: macro-in-comment %doc > You need to be very careful about trying to "comment out" rpm macros since > '#' isn't actually a comment character in a spec file. Macros are still > expanded, and if the macro happens to be multi-line then all sorts of bad > things can happen. I removed the %doc macro because that is real, the other one is a part of the mailing list URL and isn't a real macro (I hope!) so I didn't remove it. > The "for Linux" bit in your summary and description does seem kind of funny, > as this is a Fedora package so by definition it's for Linux. Not a big > deal, though. That's straight from their website (http://xwax.org/). I tend to prefer not to lose the intention of original authors, but I see what you mean. > BuildRoot: is unnecessary in Fedora, as is the "rm -rf" at the start of > %install, the entire %clean section and the %defattr line. You can remove > all of them. removed all, seems to work here still! > You don't technically need to have a build dependency on SDL-devel since > SDL_ttf-devel requires it, but this doesn't really matter. good to know but left it since it doesn't hurt. > Thanks for the rationale for not including a .desktop file. I can > understand how it wouldn't really work well, and how a menu item for this > wouldn't be useful. > > So really this only needs a few quick fixes. > > * source files match upstream. sha256sum: > 2e35aafb2a08d29c611e19a54b3e45b5c7243cf53b2aec1634c87f01fb35e6a9 > xwax-1.2.tar.gz > * package meets naming and versioning guidelines. > * specfile is properly named, is cleanly written and uses macros > consistently. > * summary is OK. > * description is OK. > * dist tag is present. > * license field matches the actual license. > * license is open source-compatible. > * license text included in package. > * latest version is being packaged. > * BuildRequires are proper. > * compiler flags are appropriate. > * package builds in mock (rawhide, x86_64). > * package installs properly. > * debuginfo package looks complete. > X rpmlint has a valid complaint > * final provides and requires are sane: > xwax = 1.2-2.fc18 > xwax(x86-64) = 1.2-2.fc18 > = > /bin/sh > cdparanoia > libSDL-1.2.so.0()(64bit) > libSDL_ttf-2.0.so.0()(64bit) > libasound.so.2()(64bit) > libasound.so.2(ALSA_0.9)(64bit) > libasound.so.2(ALSA_0.9.0rc4)(64bit) > libjack.so.0()(64bit) > sox > > * no bundled libraries. > X fails to own /usr/libexec/xwax > * doesn't own any directories it shouldn't. > X documentation is duplicated. > * file permissions are appropriate. > * no generically named files. > * code, not content. > * documentation is small, so no -doc subpackage is necessary. > * %docs are not necessary for the proper functioning of the package. > * no libtool .la files.
Great. Still builds fine, rpmlint is down to just the one spurious "macro-in-comment" complaint. The spec is clean, and the issues I had are fixed. The only other observation I might make is that it's not necessary to manually mark manpages as %doc, as rpm automatically does that for anything under %_mandir. But that's no big deal at all. APPROVED I have already sponsored you, assuming your FAS ID is "john2583". Otherwise I have to find this john2583 guy and apologize to him. After the permissions propagate (could take up to an hour) you should be able to make your SCM request and move forward with the import and build process. Feel free to catch me on #fedora-devel on IRC (I'm 'tibbs') or email me personally if you have any questions about the process.
New Package SCM Request ======================= Package Name: xwax Short Description: Open source vinyl emulation software Owners: john2583 Branches: f16 f17 InitialCC:
Git done (by process-git-requests).
xwax-1.2-3.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/xwax-1.2-3.fc17
xwax-1.2-3.fc17 has been pushed to the Fedora 17 testing repository.
xwax-1.2-3.fc17 has been pushed to the Fedora 17 stable repository.