Spec URL: https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock.spec SRPM URL: https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-3.4.0-9.fc.src.rpm Description: An light eye-candy fully themable animated dock for any Linux desktop. It has a family-likeness with OSX dock, but with more options. Fedora Account System Username: mtasaka Background: cairo-dock is now on rpmfusion. This was on Fedora, however on Feb 2009 this was removed from Fedora due to may-be-patent-infringement issue. Now with the upstream's advice I removed that "may-be" part from the original tarball. I talked with spot and this srpm got "legally okay" for Fedora. Copr: http://copr-fe.cloud.fedoraproject.org/coprs/mtasaka/cairo-dock0test/
rpmlint: cairo-dock-core.x86_64: W: obsolete-not-provided cairo-dock-plug-ins-gecko cairo-dock-core.x86_64: W: obsolete-not-provided cairo-dock-themes - These are some old age Obsoletes. cairo-dock-core.x86_64: E: incorrect-fsf-address /usr/share/licenses/cairo-dock-core/copyright - I will tell the upstream together with cairo-dock-plug-ins source later cairo-dock-devel.x86_64: W: only-non-binary-in-usr-lib - This is usual All the rest should be ignorable. Note that "cairo-dock" is a metapackage, which also depends on cairo-dock-plug-ins{,-python2} (ref: bug 1178912), however I can cope with - build cairo-dock srpm - build cairo-dock-plug-ins srpm after cairo-dock rebuilt - submit cairo-dock, cairo-dock-plug-ins to updates request altogether and no bootstrapping is needed.
Mamoru-san, I'm about to review cairo-dock{,-plug-ins}. Can you review clufter [bug 1180723] in exchange, please?
Thank you for taking. I took your review request in exchange.
Briefly: - minor BSD-licensed file discovered - some dates in changelog would deserve fixing Below I commented also on some items that are OK: > Package Review > ============== > > Legend: > [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated > [ ] = Manual review needed > > > Issues: > ======= > - 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. > Note: Cannot find LICENSE in rpm(s) > See: > http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text > - Package do not use a name that already exist > Note: A package already exist with this name, please check > https://admin.fedoraproject.org/pkgdb/acls/name/cairo-dock > See: > https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Conflicting_Package_Names I am not sure here, I suppose the existing orhpaned package will get un-retired (https://admin.fedoraproject.org/pkgdb/package/cairo-dock/) > > ===== MUST items ===== > > C/C++: > [x]: Package does not contain kernel modules. > [x]: Package contains no static executables. > [x]: Development (unversioned) .so files in -devel subpackage, if present. > Note: Unversioned so-files in private %_libdir subdirectory (see > attachment). Verify they are not in ld path. cairo-dock-core: /usr/lib64/cairo-dock/libcd-Help.so ... but this is not a development .so, and is correctly in private subdir Furthermore, CMakeList.txt makes it clear it is a special plugin for cairo-dock that cannot be packaged in extra plug-ins package. > [x]: Header files in -devel subpackage, if present. > [x]: ldconfig called in %post and %postun if required. > [x]: Package does not contain any libtool archives (.la) > [x]: Rpath absent or only used for internal libs. > > Generic: > [x]: Package is licensed with an open-source compatible license and meets > other legal requirements as defined in the legal section of Packaging > Guidelines. You mentioned there were some concerns in the past but now it was ACK'd by spot. It would be best if it there was a public record of that, couldn't find anything on fedora-legal/fedora-devel MLs. Only that it is better to have a transparency in this regard. IIUIC, the patent stuff is being removed prior to creating the resulting source tarball thanks cairo-dock-create-fedora-tarball.sh. > [!]: License field in the package spec file matches the actual license. > Note: Checking patched sources after %prep for licenses. Licenses found: > "GPL (v2 or later)", "LGPL (v2 or later)", "GPL (unversioned/unknown > version) GPL (v3)", "GPL (v3 or later)", "Unknown or generated". 13 files > have unknown license. Detailed output of licensecheck in /home/mock > /cairo-dock/1178911-cairo-dock/cairo-dock/licensecheck.txt Everything seems OK except for BSD license of cairo-dock-3.4.0/cmake_modules/GNUInstallDirs.cmake which refers to BSD license of CMake (and, as per instructions in the header, it should be included in full in that very file!), which turns to be BSD 3-Clause "New" or "Revised" license. Hence I think the license should be: GPLv3+ and BSD Alternatively, a fact that native CMake version exists can be utilized: $ rpm -qf /usr/share/cmake/Modules/GNUInstallDirs.cmake cmake-3.0.2-2.fc21.x86_64 > [ ]: License file installed when any subpackage combination is installed. This doesn't seem to be true, e.g., one may install cairo-dock-libs and miss the licenses/* files. > [x]: %build honors applicable compiler flags or justifies otherwise. Perhaps %cmake macro is robust enough so thar -O3 -> -O2 in CMakeLists.txt is not necessary? > [x]: Package contains no bundled libraries without FPC exception. > [ ]: Changelog in prescribed format. OK except rpmlint reports: cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Thu Dec 21 2011 Mamoru Tasaka <mtasaka> - 2.4.0.2-2 cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Sat Nov 6 2009 Mamoru Tasaka <mtasaka.u-tokyo.ac.jp> - 2.1.1.2-1 cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Wed May 27 2008 Mamoru Tasaka <mtasaka.u-tokyo.ac.jp> - 1.5.6-1.date20080528 > [x]: Sources contain only permissible code or content. > [x]: Each %files section contains %defattr if rpm < 4.4 > Note: %defattr present but not needed ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > [x]: Development files must be in a -devel package > [x]: Package uses nothing in %doc for runtime. > [x]: The spec file handles locales properly. > [x]: Package consistently uses macros (instead of hard-coded directory names). (/sbin/ldconfig based on https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries contradicts a bit, and there is no macro for that command) > [x]: Package is named according to the Package Naming Guidelines. > [x]: Package does not generate any conflict. > [x]: Package obeys FHS, except libexecdir and /usr/target. > [-]: If the package is a rename of another package, proper Obsoletes and > Provides are present. > [x]: Requires correct, justified where necessary. note wrt. cairo-dock, one can still install just cairo-dock-{core,libs} and then particular plugins, so this is OK > [x]: Spec file is legible and written in American English. > [-]: Package contains systemd file(s) if in need. > [x]: Useful -debuginfo package or justification otherwise. > [x]: Package is not known to require an ExcludeArch tag. > [x]: Large documentation must go in a -doc subpackage. Large could be size > (~1MB) or number of files. > Note: Documentation size is 30720 bytes in 3 files. > [x]: Package complies to the Packaging Guidelines > [x]: Package successfully compiles and builds into binary rpms on at least one > supported primary architecture. > [x]: Rpmlint is run on all rpms the build produces. > Note: There are rpmlint messages (see attachment). > [x]: Package requires other packages for directories it uses. > [x]: Package must own all directories that it creates. > [x]: Package does not own files or directories owned by other packages. > [x]: All build dependencies are listed in BuildRequires, except for any that > are listed in the exceptions section of Packaging Guidelines. > [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT > [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the > beginning of %install. > [x]: Macros in Summary, %description expandable at SRPM build time. > [x]: Package contains desktop file if it is a GUI application. > [x]: Package installs a %{name}.desktop using desktop-file-install or desktop- > file-validate if there is such a file. > [x]: Package does not contain duplicates in %files. > [x]: Permissions on files are set properly. > [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't > work. > [x]: Package is named using only allowed ASCII characters. > [x]: Package is not relocatable. > [x]: Sources used to build the package match the upstream source, as provided > in the spec URL. > [x]: Spec file name must match the spec package %{name}, in the format > %{name}.spec. > [x]: File names are valid UTF-8. > [x]: Packages must not store files under /srv, /opt or /usr/local > > ===== SHOULD items ===== > > Generic: > [ ]: If the source package does not include license text(s) as a separate file > from upstream, the packager SHOULD query upstream to include it. Related to the possible BSD issue (License field in the package spec...) > [x]: Final provides and requires are sane (see attachments). > [x]: Fully versioned dependency in subpackages if applicable. > Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in cairo-dock- > libs , cairo-dock-core , cairo-dock-devel > [x]: Package functions as described. > [x]: Latest version is packaged. > [x]: Package does not include license text files separate from upstream. > [x]: Scriptlets must be sane, if used. > [x]: SourceX tarball generation or download is documented. > Note: Package contains tarball without URL, check comments > [ ]: Description and summary sections in the package spec file contains > translations for supported Non-English languages, if available. This sound merely like nice-to-have :) > [ ]: Package should compile and build into binary rpms on all supported > architectures. Haven't verified this. > [ ]: %check is present and all tests pass. Tests are present in tests subdir, but perhaps impractical to run. > [x]: Packages should try to preserve timestamps of original installed files. > [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file > [x]: Reviewer should test that the package builds in mock. > [x]: Buildroot is not present > [x]: Package has no %clean section with rm -rf %{buildroot} (or > $RPM_BUILD_ROOT) > [x]: Dist tag is present (not strictly required in GL). > [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. > [x]: Uses parallel make %{?_smp_mflags} macro. > [x]: The placement of pkgconfig(.pc) files are correct. > [x]: SourceX is a working URL. > [x]: Spec use %global instead of %define unless justified. > Rpmlint > ------- > Checking: cairo-dock-3.4.0-9.fc21.x86_64.rpm > cairo-dock-libs-3.4.0-9.fc21.x86_64.rpm > cairo-dock-core-3.4.0-9.fc21.x86_64.rpm > cairo-dock-devel-3.4.0-9.fc21.x86_64.rpm > cairo-dock-3.4.0-9.fc21.src.rpm > cairo-dock.x86_64: W: spelling-error Summary(en_US) themable -> them able, them-able, fathomable > cairo-dock.x86_64: W: spelling-error %description -l en_US metapackage -> meta package, meta-package, prepackage > cairo-dock-core.x86_64: W: spelling-error %description -l en_US themable -> them able, them-able, fathomable > cairo-dock.x86_64: E: no-binary This cannot be noarch as then no subpackage cannot be reverted to "arch" (was experimenting with something similar for clufter). > cairo-dock.x86_64: W: no-documentation > cairo-dock-libs.x86_64: W: no-documentation > cairo-dock-devel.x86_64: W: no-documentation > cairo-dock-core.x86_64: W: obsolete-not-provided cairo-dock-plug-ins-gecko > cairo-dock-core.x86_64: W: obsolete-not-provided cairo-dock-themes justified > cairo-dock-core.x86_64: E: incorrect-fsf-address /usr/share/licenses/cairo-dock-core/copyright OK for now, to be discussed with upstream as per [comment 1]. > cairo-dock-devel.x86_64: W: only-non-binary-in-usr-lib non-versioned lib symlink to versioned one > cairo-dock.src: W: spelling-error Summary(en_US) themable -> them able, them-able, fathomable > cairo-dock.src: W: spelling-error %description -l en_US metapackage -> meta package, meta-package, prepackage not sure if there is any strong preference in other packages; themable OK (~parsable, movable). > cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Thu Dec 21 2011 Mamoru Tasaka <mtasaka> - 2.4.0.2-2 > cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Sat Nov 6 2009 Mamoru Tasaka <mtasaka.u-tokyo.ac.jp> - 2.1.1.2-1 > cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Wed May 27 2008 Mamoru Tasaka <mtasaka.u-tokyo.ac.jp> - 1.5.6-1.date20080528 might be worth fixing otherwise it will be reported over and over in any (local) rebuild, etc. > 5 packages and 0 specfiles checked; 5 errors, 11 warnings.
> Note that "cairo-dock" is a metapackage, which also depends on > cairo-dock-plug-ins{,-python2} (ref: bug 1178912), however I can cope > with > - build cairo-dock srpm > - build cairo-dock-plug-ins srpm after cairo-dock rebuilt > - submit cairo-dock, cairo-dock-plug-ins to updates request altogether > and no bootstrapping is needed. Maybe worth documenting directly in the specfile, or perhaps even better, in some MAINTAINERS.fedora file in the upstream tree. Personally, I don't have experience with such a setup (used mockchain locally) so hopefully it is acceptable and optimal.
Thank you for initial comments!! First I write some reply to your comments > I am not sure here, I suppose the existing orhpaned package will get un-retired (https://admin.fedoraproject.org/pkgdb/package/cairo-dock/) Yes. > You mentioned there were some concerns in the past but now it was ACK'd by spot. It would be best if it there was a public record of that, couldn't find anything on fedora-legal/fedora-devel MLs. Well, there is no public record. All spot and my exchange was in private. I am not sure if I can make these show in public (as this is very legal issue), so for now I put them in private (but I can write the summary of the mail, will add) > IIUIC, the patent stuff is being removed prior to creating the resulting source tarball thanks cairo-dock-create-fedora-tarball.sh. Yes. > Hence I think the license should be: GPLv3+ and BSD GPLv3+ is "more strict" than BSD (and BSD is compatible with GPL) so it is okay to write "GPLv3+" only: https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#What_is_.22effective_license.22_and_do_I_need_to_know_that_for_the_License:_tag.3F > This doesn't seem to be true, e.g., one may install cairo-dock-libs and miss the licenses/* files. Yes, this must need fixing. Will fix in the next push > Perhaps %cmake macro is robust enough so thar -O3 -> -O2 in CMakeLists.txt is not necessary? Well, for gcc compiler flags related to optimization level, the latter always wins (i.e. gcc -O2 -O3 is gcc -O3). So it is safer to change -O3 in source code to -O2 explicitly. > For %changelog dates rpmlint - Well, I don't rememeber what date is correct any longer... So although it is true that date is wrong in some way, I don't want to fix them. > Personally, I don't have experience with such a setup (used mockchain locally) so hopefully it is acceptable and optimal. Well, for this build bootstrap is not difficult. Just building in the order of cairo-dock -> cairo-dock-plug-ins (srpm) is okay. Other comments seems okay. I will fix - license files placement (should be moved to -libs) - write some brief explanation about legal issue.
>> You mentioned there were some concerns in the past but now it was ACK'd >> by spot. It would be best if it there was a public record of that, >> couldn't find anything on fedora-legal/fedora-devel MLs. > Well, there is no public record. All spot and my exchange was in private. > I am not sure if I can make these show in public (as this is very legal > issue), so for now I put them in private (but I can write the summary > of the mail, will add) Oh, don't want to cause more issues as opposed to just have somehow confirmed any previous legal issue was overcome successfully. That being said, Tom/spot, can you chime in to verify the statement, please? (The only reference I could find is https://fedoraproject.org/w/index.php?title=Deprecated_packages&oldid=327569 ).
Yes, Mamoru has done the necessary work in conjunction with upstream to resolve the legal issues which blocked this package from Fedora before.
Thanks, spot! Mamoru, some more points below: Regarding the License tag vs BSD: there was an apparent mistake on my side as I was considering all files within a source RPM, not binary RPMs. "Better safe than sorry", but needless exercise (well, not sure how common/sane is to relay on customizations of standard CMake modules, but this is an upstream question). >> For %changelog dates rpmlint > - Well, I don't rememeber what date is correct any longer... > So although it is true that date is wrong in some way, I don't want to > fix them. Accepted, I only wanted to point out that it is relatively a new feature of RPM to complain any time a specfile with iffy dates is being parsed and that this dirt under the carpet will keep exposing itself over and over. Could I please know your standpoint on: - %check phase - documenting the build order wrt. cairo-dock-plug-ins - side-question: will this get right automatically during mass rebuilds? In the light of this review, I'll jump briefly on cairo-dock-plug-ins part.
Well %check - There are some test scripts under tests, however this depends on -plug-ins, and moreover proper dbus session needs running, so I don't think it is suitable. - Build order - Not difficult and I don't think it needs documentation. For mass rebuild no build order need consideration.
https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock.spec https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-3.4.0-10.fc.src.rpm * Tue Jan 20 2015 Mamoru TASAKA <mtasaka> - 3.4.0-10 - Move license files to -libs - Add an explanation text for legal issue
Looks good to me except for one new thing regarding core/libs split: /usr/share/cairo-dock/scripts/cairo-dock-package-theme.sh should IMHO be packaged in cairo-dock-libs binary RPM, which contains the user of this script (libgldi.so*) When the split is in place, it's reasonable to make it follow the actual inter-dependencies. There may be more such cases, but that can be figured out later on, it should be quite rare.
(In reply to Jan Pokorný from comment #12) > Looks good to me except for one new thing regarding core/libs split: > > /usr/share/cairo-dock/scripts/cairo-dock-package-theme.sh should IMHO be > packaged in cairo-dock-libs binary RPM, which contains the user of > this script (libgldi.so*) > > When the split is in place, it's reasonable to make it follow the actual > inter-dependencies. There may be more such cases, but that can be figured > out later on, it should be quite rare. -libs usually contains "libraries" only avoid multilib issue when installing -devel package and does not contain any runtime files.
OK, not disputing it then. ACK, -plug-ins review is in the works.
(After-the-fact) suggestion: - %defattr(-,root,root,-) should not be needed as it is a default (similar to %doc/[1180723 comment 12]).
([bug 1180723 comment 12])
(After-the-fact) suggestion #2: > Packages containing graphical applications should include AppData files. https://fedoraproject.org/wiki/Packaging:AppData
Thank you! For appdata, I will ask the upstream first. For SCM request, I will do once -plug-ins is also approved.
Since plug-ins review also passed (bug 1178912), now I do scm request Un-orphaning request Package Change Request ====================== Package Name: cairo-dock New Branches: devel f22 f21 f20 Owners: mtasaka InitialCC:
Git done (by process-git-requests). Unretired.
Rebuilt on all branches, push request on F-22/21/20, now closing. Thank you for review and git preocedure.