Spec URL:http://slaanesh.fedorapeople.org/libguac.spec SRPM URL: http://slaanesh.fedorapeople.org/libguac-0.6.0-1.fc17.src.rpm Description: This is the core Guacamole library on which the Guacamole daemon and all client plugins depend. ****** Note: Part of the Guacamole package review http://guac-dev.org/
Please see http://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net . It seems like the GPL part of the license is GPLv2+. You could consider including doxygen documentation in the devel package. I guess a real review of this require a package that actually use it.
Hi, many thanks for the review, here are the updates: 1- Corrected the Sourceforge URLs. 2- Corrected the license to all the packages to GPLv2+; not only this one but all the Guacamole packages are GPLv2+. All have been re-uploaded. 3- I'm looking at the docs now, and see what I can do. 4- I've added now the review request for guacd, the Guacamole daemon at: https://bugzilla.redhat.com/show_bug.cgi?id=820561 The package has init files for both RHEL/CentOS 6 and Fedora with conditionals and uses libguac. Thanks, --Simone
I've added doxygen generated output to the devel package.
I will take this review.
Some remarks: * Please increment the Release-tag each time you modify the submission. This helps reviewers to track changes during a package's review. * (BLOCKER) The Source0-URL doesn't work. http://downloads.sourceforge.net/guacamole/libguac-0.6.0.tar.gz seems to work. * (REMARK) The package seems is passing -Werror -pedantic in its CFLAGS. This will render maintaining this package as part of a distribution hard. (-Werror causes GCC to raises warnings to errors and -pedantic raises warnings on "bizarre cases". Both together will cause architecture and gcc-version dependent build-breakdowns.) * (REMARK) In %install, rm -rf $RPM_BUILD_ROOT is not required anymore on RHEL6 or current Fedoras.
Thanks, I've added the required changes: Spec URL:http://slaanesh.fedorapeople.org/libguac.spec SRPM URL: http://slaanesh.fedorapeople.org/libguac-0.6.0-2.fc17.src.rpm * Release tag incremented. * Source0 URL corrected, I followed the guidelines at: fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net but forgot to put the project name. * Removed rm -fr in %install, i will only build the package in RHEL6+ and Fedora. The commands are generated by "rpmdev-newspec", so I guess it's a bug here. * Added CFLAGS="$RPM_OPT_FLAGS" to the build section to override CFLAGS.
A few pre-review remarks: * The first line in %build, "export CFLAGS="$RPM_OPT_FLAGS", is unnecessary. The %configure macro does the same thing. See the expansion of that macro in /usr/lib/rpm/macros. * I question the usefulness of including the latex directory in %doc for the -devel subpackage. The LaTeX files are source files for the documentation. They aren't really readable by a developer wanting to use this library. For that, you want to convert them into some format targeted at a human, such as PDF or HTML. Since you're already building and packaging the HTML documentation, I see no utility in including the LaTeX sources, too. * The configure run includes this line: checking for clock_gettime... no which is wrong, because Linux does have clock_gettime(), but you have to link with -lrt. If upstream will add AC_CHECK_LIB([rt], [clock_gettime]) to configure.in, this will take care of itself. In the meantime, adding this to %prep does the trick: sed -i '/clock_gettime/iLIBS+=" -lrt"' configure * Running rpmlint on the install libguac-devel package (not the rpm) says: libguac-devel.x86_64: W: doc-file-dependency /usr/share/doc/libguac-devel-0.6.0/html/installdox /usr/bin/perl You probably don't want to install the installdox script.
Package Review ============== Key: - = N/A x = Pass ! = Fail ? = Not evaluated ==== C/C++ ==== [x]: MUST Header files in -devel subpackage, if present. [x]: MUST ldconfig called in %post and %postun if required. [x]: MUST Package does not contain any libtool archives (.la) [x]: MUST Package does not contain kernel modules. [x]: MUST Package contains no static executables. [x]: MUST Rpath absent or only used for internal libs. [x]: MUST Package is not relocatable. [x]: MUST Development (unversioned) .so files in -devel subpackage, if present. ==== Generic ==== [x]: MUST Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: MUST Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: MUST %build honors applicable compiler flags or justifies otherwise. [x]: MUST All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: MUST Buildroot is not present Note: Unless packager wants to package for EPEL5 this is fine [x]: MUST Package contains no bundled libraries. [x]: MUST Changelog in prescribed format. [x]: MUST Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) Note: Clean would be needed if support for EPEL is required [x]: MUST Sources contain only permissible code or content. [x]: MUST Each %files section contains %defattr if rpm < 4.4 Note: Note: defattr macros not found. They would be needed for EPEL5 [x]: MUST Macros in Summary, %description expandable at SRPM build time. [x]: MUST Package requires other packages for directories it uses. [x]: MUST Package uses nothing in %doc for runtime. [x]: MUST Package is not known to require ExcludeArch. [x]: MUST Permissions on files are set properly. [x]: MUST Package does not contain duplicates in %files. [x]: MUST Fully versioned dependency in subpackages, if present. [x]: MUST Spec file lacks Packager, Vendor, PreReq tags. [x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. Note: rm -rf would be needed if support for EPEL5 is required [x]: MUST Large documentation files are in a -doc subpackage, if required. [x]: 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. [x]: MUST License field in the package spec file matches the actual license. [x]: MUST License file installed when any subpackage combination is installed. [x]: MUST Package consistently uses macros (instead of hard-coded directory names). [x]: MUST Package is named according to the Package Naming Guidelines. [x]: MUST Package does not generate any conflict. [x]: MUST Package obeys FHS, except libexecdir and /usr/target. [x]: MUST Package must own all directories that it creates. [x]: MUST Package does not own files or directories owned by other packages. [x]: MUST Package installs properly. [x]: MUST Requires correct, justified where necessary. [!]: MUST Rpmlint output is silent. rpmlint libguac-0.6.0-2.fc18.i686.rpm libguac.i686: W: spelling-error %description -l en_US guacd -> guard 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint libguac-0.6.0-2.fc18.src.rpm libguac.src: W: spelling-error %description -l en_US guacd -> guard 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint libguac-devel-0.6.0-2.fc18.i686.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint libguac-debuginfo-0.6.0-2.fc18.i686.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. [x]: MUST Sources used to build the package match the upstream source, as provided in the spec URL. /home/jamesjer/820542/libguac-0.6.0.tar.gz : MD5SUM this package : 37e079dbe081123d0e72b4cacb9698fd MD5SUM upstream package : 37e079dbe081123d0e72b4cacb9698fd [x]: MUST Spec file is legible and written in American English. [x]: MUST Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: MUST Package contains a SysV-style init script if in need of one. [x]: MUST File names are valid UTF-8. [x]: MUST Useful -debuginfo package or justification otherwise. [x]: SHOULD Reviewer should test that the package builds in mock. [-]: 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. [x]: SHOULD Dist tag is present. [x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [x]: SHOULD Package functions as described. [x]: SHOULD Latest version is packaged. [x]: SHOULD Package does not include license text files separate from upstream. [x]: SHOULD Scriptlets must be sane, if used. [x]: SHOULD SourceX is a working URL. [x]: SHOULD Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: SHOULD Package should compile and build into binary rpms on all supported architectures. [-]: SHOULD %check is present and all tests pass. Upstream did not include any tests. [!]: SHOULD Packages should try to preserve timestamps of original installed files. See below for how to fix this. [x]: SHOULD Spec use %global instead of %define. Issues: [!]: MUST Rpmlint output is silent. rpmlint libguac-0.6.0-2.fc18.i686.rpm libguac.i686: W: spelling-error %description -l en_US guacd -> guard 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint libguac-0.6.0-2.fc18.src.rpm libguac.src: W: spelling-error %description -l en_US guacd -> guard 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint libguac-devel-0.6.0-2.fc18.i686.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint libguac-debuginfo-0.6.0-2.fc18.i686.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint [!]: SHOULD Packages should try to preserve timestamps of original installed files. To fix this, change the first line of %install to this: make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p" Generated by fedora-review 0.1.3 External plugins:
(In reply to comment #7) > A few pre-review remarks: > > * The first line in %build, "export CFLAGS="$RPM_OPT_FLAGS", is unnecessary. Agreed. It also doesn't help wrt. the "-Werror -pedantic" issue, because this package has "-Werror -Wall -pedantic" hard-coded into its Makefile.am. The ultimate fix would be upstream to remove it there. There'd be other ways to work around this locally, but for now, there is no need for any action for Fedora. > * The configure run includes this line: > > checking for clock_gettime... no An easier way is to append LIBS to %configure: %configure --disable-static LIBS="-lrt" instead of modifying configure.
Spec URL:http://slaanesh.fedorapeople.org/libguac.spec SRPM URL: http://slaanesh.fedorapeople.org/libguac-0.6.0-3.fc17.src.rpm Hello, thanks for the inputs. I've added the following: - Removed generated latex files from doxygen into libguac-devel. - Added LIBS="-lrt" to configure. - Added to first line of %install: INSTALL="install -p" - Removed CFLAGS; I will ask upstream post-review to implement all changes resulted from the package reviews for all packages in the Guacamole stack. I don't know how to change the following though, it's the package name: $ rpmlint libguac-* libguac.src: W: spelling-error %description -l en_US guacd -> guard libguac.x86_64: W: spelling-error %description -l en_US guacd -> guard 4 packages and 0 specfiles checked; 0 errors, 2 warnings. And this: > * Running rpmlint on the install libguac-devel package (not the rpm) says: > > libguac-devel.x86_64: W: doc-file-dependency > /usr/share/doc/libguac-devel-0.6.0/html/installdox /usr/bin/perl > > You probably don't want to install the installdox script. There's no "installdox" script packaged into libguac-devel, what does it mean? Thanks, --Simone
(In reply to comment #10) > I don't know how to change the following though, it's the package name: > > $ rpmlint libguac-* > libguac.src: W: spelling-error %description -l en_US guacd -> guard > libguac.x86_64: W: spelling-error %description -l en_US guacd -> guard > 4 packages and 0 specfiles checked; 0 errors, 2 warnings. You can't change it. Just ignore it. The spell check is intended to catch common English spelling errors. The dictionary does not contain project-specific words, or even very many technical words. > There's no "installdox" script packaged into libguac-devel, what does it mean? It is /usr/share/doc/libguac-devel-0.6.0/html/installdox. Add this to the end of your %install section to remove it: rm -f html/installdox I trust you to make that change before importing the package. As all MUST items pass inspection, this package is APPROVED.
Many thanks for the review! I'm sorry but I don't get the "installdox" point. There's no installdox file packaged into the rpms; even if I add rm -f I'm not removing anything: $ rpm -qpd libguac-devel-0.6.0-3.fc17.x86_64.rpm | grep html /usr/share/doc/libguac-devel-0.6.0/html/annotated.html /usr/share/doc/libguac-devel-0.6.0/html/bc_s.png /usr/share/doc/libguac-devel-0.6.0/html/bdwn.png /usr/share/doc/libguac-devel-0.6.0/html/classes.html /usr/share/doc/libguac-devel-0.6.0/html/client_8h.html /usr/share/doc/libguac-devel-0.6.0/html/client_8h_source.html /usr/share/doc/libguac-devel-0.6.0/html/closed.png /usr/share/doc/libguac-devel-0.6.0/html/doxygen.css /usr/share/doc/libguac-devel-0.6.0/html/doxygen.png /usr/share/doc/libguac-devel-0.6.0/html/error_8h.html /usr/share/doc/libguac-devel-0.6.0/html/error_8h_source.html /usr/share/doc/libguac-devel-0.6.0/html/files.html /usr/share/doc/libguac-devel-0.6.0/html/functions.html /usr/share/doc/libguac-devel-0.6.0/html/functions_vars.html /usr/share/doc/libguac-devel-0.6.0/html/globals.html /usr/share/doc/libguac-devel-0.6.0/html/globals_defs.html /usr/share/doc/libguac-devel-0.6.0/html/globals_enum.html /usr/share/doc/libguac-devel-0.6.0/html/globals_eval.html /usr/share/doc/libguac-devel-0.6.0/html/globals_func.html /usr/share/doc/libguac-devel-0.6.0/html/globals_type.html /usr/share/doc/libguac-devel-0.6.0/html/globals_vars.html /usr/share/doc/libguac-devel-0.6.0/html/index.html /usr/share/doc/libguac-devel-0.6.0/html/jquery.js /usr/share/doc/libguac-devel-0.6.0/html/nav_f.png /usr/share/doc/libguac-devel-0.6.0/html/nav_h.png /usr/share/doc/libguac-devel-0.6.0/html/open.png /usr/share/doc/libguac-devel-0.6.0/html/palette_8h_source.html /usr/share/doc/libguac-devel-0.6.0/html/protocol_8h.html /usr/share/doc/libguac-devel-0.6.0/html/protocol_8h_source.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_61.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_61.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_63.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_63.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_64.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_64.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_65.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_65.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_66.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_66.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_67.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_67.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_68.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_68.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_69.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_69.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_6b.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_6b.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_6c.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_6c.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_6d.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_6d.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_6f.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_6f.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_70.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_70.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_73.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_73.js /usr/share/doc/libguac-devel-0.6.0/html/search/all_76.html /usr/share/doc/libguac-devel-0.6.0/html/search/all_76.js /usr/share/doc/libguac-devel-0.6.0/html/search/classes_67.html /usr/share/doc/libguac-devel-0.6.0/html/search/classes_67.js /usr/share/doc/libguac-devel-0.6.0/html/search/close.png /usr/share/doc/libguac-devel-0.6.0/html/search/defines_67.html /usr/share/doc/libguac-devel-0.6.0/html/search/defines_67.js /usr/share/doc/libguac-devel-0.6.0/html/search/enums_67.html /usr/share/doc/libguac-devel-0.6.0/html/search/enums_67.js /usr/share/doc/libguac-devel-0.6.0/html/search/enumvalues_67.html /usr/share/doc/libguac-devel-0.6.0/html/search/enumvalues_67.js /usr/share/doc/libguac-devel-0.6.0/html/search/files_63.html /usr/share/doc/libguac-devel-0.6.0/html/search/files_63.js /usr/share/doc/libguac-devel-0.6.0/html/search/files_65.html /usr/share/doc/libguac-devel-0.6.0/html/search/files_65.js /usr/share/doc/libguac-devel-0.6.0/html/search/files_70.html /usr/share/doc/libguac-devel-0.6.0/html/search/files_70.js /usr/share/doc/libguac-devel-0.6.0/html/search/files_73.html /usr/share/doc/libguac-devel-0.6.0/html/search/files_73.js /usr/share/doc/libguac-devel-0.6.0/html/search/functions_67.html /usr/share/doc/libguac-devel-0.6.0/html/search/functions_67.js /usr/share/doc/libguac-devel-0.6.0/html/search/functions_76.html /usr/share/doc/libguac-devel-0.6.0/html/search/functions_76.js /usr/share/doc/libguac-devel-0.6.0/html/search/mag_sel.png /usr/share/doc/libguac-devel-0.6.0/html/search/nomatches.html /usr/share/doc/libguac-devel-0.6.0/html/search/search.css /usr/share/doc/libguac-devel-0.6.0/html/search/search.js /usr/share/doc/libguac-devel-0.6.0/html/search/search_l.png /usr/share/doc/libguac-devel-0.6.0/html/search/search_m.png /usr/share/doc/libguac-devel-0.6.0/html/search/search_r.png /usr/share/doc/libguac-devel-0.6.0/html/search/typedefs_67.html /usr/share/doc/libguac-devel-0.6.0/html/search/typedefs_67.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_61.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_61.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_63.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_63.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_64.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_64.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_66.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_66.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_67.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_67.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_68.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_68.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_69.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_69.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_6b.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_6b.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_6c.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_6c.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_6d.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_6d.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_6f.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_6f.js /usr/share/doc/libguac-devel-0.6.0/html/search/variables_73.html /usr/share/doc/libguac-devel-0.6.0/html/search/variables_73.js /usr/share/doc/libguac-devel-0.6.0/html/socket_8h.html /usr/share/doc/libguac-devel-0.6.0/html/socket_8h_source.html /usr/share/doc/libguac-devel-0.6.0/html/structguac__client.html /usr/share/doc/libguac-devel-0.6.0/html/structguac__client__plugin.html /usr/share/doc/libguac-devel-0.6.0/html/structguac__instruction.html /usr/share/doc/libguac-devel-0.6.0/html/structguac__layer.html /usr/share/doc/libguac-devel-0.6.0/html/structguac__palette.html /usr/share/doc/libguac-devel-0.6.0/html/structguac__palette__entry.html /usr/share/doc/libguac-devel-0.6.0/html/structguac__socket.html /usr/share/doc/libguac-devel-0.6.0/html/tab_a.png /usr/share/doc/libguac-devel-0.6.0/html/tab_b.png /usr/share/doc/libguac-devel-0.6.0/html/tab_h.png /usr/share/doc/libguac-devel-0.6.0/html/tab_s.png /usr/share/doc/libguac-devel-0.6.0/html/tabs.css Regards, --Simone
New Package SCM Request ======================= Package Name: libguac Short Description: The common library used by all C components of Guacamole Owners: slaanesh Branches: f16 f17 el6 InitialCC:
Git done (by process-git-requests).