Spec URL: http://cwickert.fedorapeople.org/review/lxappearance-obconf.spec SRPM URL: http://cwickert.fedorapeople.org/review/lxappearance-obconf-0.1.0-0.1.20100903git1769cdca.fc15.src.rpm Description: This plugin adds an additional tab called "Window Border" to LXAppearance. It is only visible when the plugin is installed and Openbox is in use.
This builds OK for me. I know that I haven't a hope of testing this, though, so hopefully someone who can will take a look. rpmlint complains about all of the macros in comments, which I don't think is problematic. (If those macros were multi-line it would be a different story, of course.) The changelog seems to be out of date when compared with the package. Normally I'd ask if you would update the snapshot before any kind of review, but it doesn't appear that there's been any non-translation-related upstream commits since that snapshot was taken.
Hi Jason, thanks for picking up this review. You are right, the changelog was not only not up to date but also incorrect. I fixed it and updated the package to latest git (only translation updates). SRPM: http://cwickert.fedorapeople.org/review/lxappearance-obconf-0.1.0-0.1.20110128git710ba0e6.fc15.src.rpm SPEC: http://cwickert.fedorapeople.org/review/lxappearance-obconf.spec
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3029549 $ rpmlint -v * lxappearance-obconf.i686: I: checking lxappearance-obconf.i686: W: spelling-error Summary(en_US) Plugin -> Plug in, Plug-in, Plugging lxappearance-obconf.i686: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging lxappearance-obconf.i686: I: checking-url http://lxde.org/ (timeout 10 seconds) lxappearance-obconf.src: I: checking lxappearance-obconf.src: W: spelling-error Summary(en_US) Plugin -> Plug in, Plug-in, Plugging lxappearance-obconf.src: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging lxappearance-obconf.src: I: checking-url http://lxde.org/ (timeout 10 seconds) lxappearance-obconf.src:11: W: macro-in-comment %{name} lxappearance-obconf.src:12: W: macro-in-comment %{name} lxappearance-obconf.src:13: W: macro-in-comment %{git_short} lxappearance-obconf.src:23: W: macro-in-comment %{name} lxappearance-obconf.src:23: W: macro-in-comment %{version} lxappearance-obconf.src: W: invalid-url Source0: lxappearance-obconf-20110128git710ba0e6.tar.bz2 lxappearance-obconf-debuginfo.i686: I: checking lxappearance-obconf-debuginfo.i686: I: checking-url http://lxde.org/ (timeout 10 seconds) lxappearance-obconf.spec:11: W: macro-in-comment %{name} lxappearance-obconf.spec:12: W: macro-in-comment %{name} lxappearance-obconf.spec:13: W: macro-in-comment %{git_short} lxappearance-obconf.spec:23: W: macro-in-comment %{name} lxappearance-obconf.spec:23: W: macro-in-comment %{version} lxappearance-obconf.spec: W: invalid-url Source0: lxappearance-obconf-20110128git710ba0e6.tar.bz2 3 packages and 1 specfiles checked; 0 errors, 16 warnings. The warnings can be thrown away. The content of the comments will be used in the future to replace the messy appearance of this spec, because there was no release yet. In general, it looks proper regarding the build process. It could be updated to a newer Git snapshot, however. There were some translation commits in the meantime.
Christoph, Are you still wanting to package this? I'll volunteer to review it if so. (and perhaps a review swap if you have the time!) Thanks, Richard
Sure, but it depends on how complex your package is because I am really busy ATM. I have updated the package once again: SPEC: http://cwickert.fedorapeople.org/review/lxappearance-obconf.spec SRPM: http://cwickert.fedorapeople.org/review/lxappearance-obconf-0.1.0-0.1.20110714git3a0fd02d.fc16.src.rpm
Don't worry about mine, I managed a review swap for them. I'll go ahead and take this one.
Hmm... I wonder what it would take to update the Review Request template to include what branches the package is expected to be released for. That way it would be easier to know if things like: BuildRoot, rm -rf $RPM_BUILDROOT in %install, %clean, and %{defattr... are needed in the spec file. Are you planning on any EPEL releases? Thanks, Richard
(In reply to comment #7) > Are you planning on any EPEL releases? Not really, I don't consider LXDE mature enough although people urge me to release it. Nevertheless I usually keep rm -rf $RPM_BUILDROOT etc in the specs for backwards compatibility.
Bear with me, I'm still trying to figure out the intricacies of licensing but from the COPYING file I would think this should be LGPLv2, see comment from Fedora license matrix: "Note that this license was originally referred to as the GNU Library General Public License v2, but all current versions (v2.1 or newer) of the license are correctly known as the GNU Lesser General Public License."
The quote is about the term "GNU Library General Public License" vs. "GNU Lesser General Public License", but this package is definitely GPL: - It's not a library or does not include libs. Unless explicitly stated there is no reason to assume it's LGPL - COPYING is GPLv2 - src/*.c say it's GPLv2 or later, so GPLv2+
Ack! I don't think I'm ever going to get this without getting my law degree :) Perhaps the License wiki needs to be updated to be more clear :) Library and Lesser both starting with L makes things confusing (not that GNU licenses need any help in that department.) Ok, I'm almost done going through the checklist.
+: OK -: must be fixed =: should be fixed (at your discretion) ?: Question or clairification needed N: not applicable MUST: [+] rpmlint output: shown in comment: No major issues. [+] follows package naming guidelines (assuming this is a pre-release package) [+] spec file base name matches package name [+] package meets the packaging guidelines [+] package uses a Fedora approved license: GPLv2+ [+] license field matches the actual license. [+] license file is included in %doc [+] spec file is in American English [+] spec file is legible [+] sources match upstream: recursive diff of git checkout against source produced no output. [+] package builds on at least one primary arch: Tested F14 x86_64 and F15 x86_64 [N] appropriate use of ExcludeArch [+] all build requirements in BuildRequires [+] spec file handles locales properly [N] ldconfig in %post and %postun [+] no bundled copies of system libraries [N] no relocatable packages [+] package owns all directories that it creates [+] no files listed twice in %files [+] proper permissions on files [+] consistent use of macros [+] code or permissible content [N] large documentation in -doc [+] no runtime dependencies in %doc [N] header files in -devel [N] static libraries in -static [N] .so in -devel [N] -devel requires main package [+] package contains no libtool archives [N] package contains a desktop file, uses desktop-file-install/validate [+] package does not own files/dirs owned by other packages [+] all filenames in UTF-8 SHOULD: [N] query upstream for license text [N] description and summary contains available translations [+] package builds in mock [+] package builds on all supported arches [?] package functions as described: I haven't had a chance to test the package. [+] sane scriptlets [N] subpackages require the main package [N] placement of pkgconfig files [N] file dependencies versus package dependencies [N] package contains man pages for binaries/scripts I have remotely installed the package but have not had a chance to test it on my one LXDE box (MythTV Box). I'm going to assume (unless you say otherwise) that you've tested the functionality of the current git version and call this approved!
(In reply to comment #11) > Perhaps the License wiki needs to be updated to be more clear :) Library and > Lesser both starting with L makes things confusing (not that GNU licenses need > any help in that department.) I think "L as in library" really helps. Some rules of thumb: 1. GPL is more restrictive than LGPL because LGPL allows binaries that erternally link the LGPL'ed libraries to have their own license, even a proprietary one. 2. When mixing licenses, the most restrictive one applies. Code that links both LGPL and GPL can no longer be LGPL. 3. If only one binary is created of different licenses, the package's license tag is the more restrictive license. If however binaries and libraries are build, the license tag of the package must mention both licenses, e.g. "GPLv2+ and LGPLv2+". 4. If yo uare unsure, look at the headers of the code itself.
> I'm going to assume (unless you say otherwise) > that you've tested the functionality of the current git version and call this > approved! Thanks for the review. I have tested this for quite a while and it always worked. In fact not much has changed other than translation updates. New Package SCM Request ======================= Package Name: lxappearance-obconf Short Description: Plugin to configure Openbox inside LXAppearance Owners: cwickert Branches: f14 f15 InitialCC:
Git done (by process-git-requests).
lxappearance-obconf-0.1.0-0.1.20110714git3a0fd02d.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/lxappearance-obconf-0.1.0-0.1.20110714git3a0fd02d.fc15
lxappearance-obconf-0.1.0-0.1.20110714git3a0fd02d.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/lxappearance-obconf-0.1.0-0.1.20110714git3a0fd02d.fc14
lxappearance-obconf-0.1.0-0.1.20110714git3a0fd02d.fc15 has been pushed to the Fedora 15 stable repository.
lxappearance-obconf-0.1.0-0.1.20110714git3a0fd02d.fc14 has been pushed to the Fedora 14 stable repository.