Spec URL: http://slaanesh.fedorapeople.org/guacd.spec SRPM URL: http://slaanesh.fedorapeople.org/guacd-0.6.0-1.fc17.src.rpm Description: The proxy daemon which translates between remote desktop protocols and the Guacamole protocol ****** Note: Part of the Guacamole package review http://guac-dev.org/
Shouldn't the requirement to libguac-devel be strictly versioned? Or at least isa'ed? Is this usable without any of the plugins? Are they fully optional? I think it would be nice to have a short explanation of the role of each package in its descripton. Something someone not yet familiar with guacamole can understand.
1) You're right about the version required for building, I've updated all the spec files that require libguac-devel. I've not put an %{_isa} as the libraries for the correct architecture gets pulled in automatically by yum for compiled objects. The _isa tag is needed only for anything that cannot be reconnected to a specific arch package. 2) guacd is usable as it as long as you write a client for it; as suggested on the website. They provide a web interface but you can use anything. I'm still finishing packaging the web interface provided by Guacamole, I have some doubts and I need some clarification from the packaging mailing list: guacamole - The main web application, written in Java. guacamole-common - The Java API used by the web application. guacamole-common-js - The JavaScript library used by the web application. guacamole-ext - Common interfaces for extending the main web application. If that's possible I would prefer into having all the base components working correctly before venturing into java/maven/tomcat. 3) I've expanded all the descriptions for the packages with the actual descriptions taken from the main page. Thanks, --Simone
(In reply to comment #2) > I've not put an %{_isa} as the libraries for the correct architecture gets > pulled in automatically by yum for compiled objects. The _isa tag is needed > only for anything that cannot be reconnected to a specific arch package. I don't follow you there. On a multilib system with non-isa build requirement you can have foo-devel.i686 when building for x86_64 - and that will fail. It seems like it would be simpler if upstream shipped all components in one package. ;-)
(In reply to comment #3) > I don't follow you there. On a multilib system with non-isa build requirement > you can have foo-devel.i686 when building for x86_64 - and that will fail. Not all packages are always built on multiarch systems with all the targets. For this particular setup I would go only with the native libraries and executables as there's no point in having an i686 package on x86_64. From what I've seen, most of the packages are done this way in Fedora: %package name Summary: blah blah blah Requires: something-devel And they have instead _isa requirements for packages that do not include any binary, like the devel packages that usually contain only a symlink: %package devel Summary: Development files for %{name} Requires: %{name}%{?_isa} = %{version}-%{release} Should I change all the BuildRequires that I have in all the packages I mantain? As an example, here is a long BuildRequires section: http://pkgs.fedoraproject.org/gitweb/?p=bacula.git;a=blob;f=bacula.spec;h=7454def475d46071a05566260e10c61d698d8c26;hb=HEAD > It seems like it would be simpler if upstream shipped all components in one > package. ;-) Well, they also shipped binary packages for Fedora but kept all the spec files for themselves :D Thanks for your time. --Simone
(In reply to comment #4) > (In reply to comment #3) > > I don't follow you there. On a multilib system with non-isa build requirement > > you can have foo-devel.i686 when building for x86_64 - and that will fail. > > Not all packages are always built on multiarch systems with all the targets. > For this particular setup I would go only with the native libraries and > executables as there's no point in having an i686 package on x86_64. Other people might however have different setups where they do have multilib - and that should work too. > From what I've seen, most of the packages are done this way in Fedora: > > %package name > Summary: blah blah blah > Requires: something-devel > > And they have instead _isa requirements for packages that do not include any > binary, like the devel packages that usually contain only a symlink: > > %package devel > Summary: Development files for %{name} > Requires: %{name}%{?_isa} = %{version}-%{release} I think the observation is correct, but I doubt there is the causality and intent that you state. > Should I change all the BuildRequires that I have in all the packages I > mantain? IMO the guidelines are a bit too vague here. They can't describe all potential problems, but I think they should be more explicit that explicit isa must be specified if the platform must match. Non-isa requirements should IMO only be used in cases where the required package is noarch or exposes a noarch "api". Many existing packages have potential problems in this area, but it would be nice to not introduce it in new places.
Updated all packages I'm requesting review for with the _isa tag in the BuildRequires. Anything else to make this packages approved? fedora-review tool? --Simone
libguac on which this package is dependant on has been approved. --Simone
Some comments: - the mkdir lines is not needed ( the -D to install will fix dir creation). - could you do a koji scratch build (I think BuildRequires: systemd-units is needed for unitdir macro). - you also need more Requires to let systemctl or chkconfig be available during install: chkconfig: http://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscripts_in_spec_file_scriptlets systemd: Requires(post): systemd-sysv Requires(post): systemd-units Requires(preun): systemd-units Requires(postun): systemd-units - you change between %{name} macro and plain guacd at random, please keep %{name} usage to minimum (if at all). - you also mix install and %{__install} macro, stick to one form only. - don't use .gz in %{_mandir}/man8/%{name}.8.gz let rpm decide compression and just use ...8.*
Thanks for all the input, here's an updated package: Spec URL: http://slaanesh.fedorapeople.org/guacd.spec SRPM URL: http://slaanesh.fedorapeople.org/guacd-0.6.0-2.fc17.src.rpm - Used common macro styles (buildroot, name). - Removed macros for base commands as per package guidelines. - Added missing BuildRequires. - Removed compression for manpages.
This is not building in mock, missing systemd-units in buildreq? RPM build errors: File must begin with "/": %{_unitdir}/guacd.service
That's strange, I build everything in mock and it builds fine for me f17/f18. I had it in place but removed it since it built fine. Is that required up to a particular version? (i.e. up to f16?) Here it is with the BR re-added: Spec URL: http://slaanesh.fedorapeople.org/guacd.spec SRPM URL: http://slaanesh.fedorapeople.org/guacd-0.6.0-3.fc17.src.rpm Thanks, --Simone
Tested with koji and f16, it builds fine also on f16 without a BR on systemd-units.
Sorry, yes it's required, I had a build that was pulling it in as a dependency. 0.6.0-3 in comment #11 is correct. Many thanks, --Simone
==== C/C++ ==== [x]: MUST Header files in -devel subpackage, if present. [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. ==== 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. MPLv1.1 or GPLv2+ or LGPLv2+ is correct. [x]: MUST Package successfully compiles and builds into binary rpms on at least one supported primary architecture. x86_64 is good. [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 %config files are marked noreplace or the reason is justified. [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 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. [?]: MUST License field in the package spec file matches the actual license. See comments in bugzilla #820543 [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 No %config files under /usr. [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. [x]: MUST Rpmlint output is silent. rpmlint guacd-0.6.0-3.fc16.x86_64.rpm guacd.x86_64: E: script-without-shebang /lib/systemd/system/guacd.service rpmlint guacd-0.6.0-3.fc16.src.rpm guacd.src:18: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 18) 1 packages and 0 specfiles checked; 0 errors, 1 warnings. [x]: MUST Sources used to build the package match the upstream source, as provided in the spec URL. /fedora/820561/guacd-0.6.0.tar.gz : MD5SUM this package : cfcddc7c466846ff886bf67248564d1a MD5SUM upstream package : cfcddc7c466846ff886bf67248564d1a [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/systemd 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. See comments in bugzilla #820543. [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. x86_64 is fine. [x]: SHOULD %check is present and all tests pass. [x]: SHOULD Packages should try to preserve timestamps of original installed files. [x]: SHOULD Spec use %global instead of %define. Issues: rpmlint guacd-0.6.0-3.fc16.x86_64.rpm guacd.x86_64: E: script-without-shebang /lib/systemd/system/guacd.service rpmlint guacd-0.6.0-3.fc16.src.rpm guacd.src:18: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 18) Summary: - systemd servince files is not scripts, mode should be 0644. - lines starting with Requires(post): is using tab, not white space, fix that. - licences text missing as in #820543
(In reply to comment #14) > Summary: > - systemd servince files is not scripts, mode should be 0644. > - lines starting with Requires(post): is using tab, not white space, fix > that. Applied all changes here: Spec URL: http://slaanesh.fedorapeople.org/guacd.spec SRPM URL: http://slaanesh.fedorapeople.org/guacd-0.6.0-4.fc17.src.rpm > - licences text missing as in #820543 This, along with all the other packages, will go into a request for upstream to include the license files for all the valid licenses and all the other small notes discovered during the reviews. Thanks, --Simone
Thanks for quick fixes. Package guacd is APPROVED.
New Package SCM Request ======================= Package Name: guacd Short Description: Proxy daemon for Guacamole Owners: slaanesh Branches: f17 f16 el6 InitialCC:
(In reply to comment #16) > Thanks for quick fixes. > > Package guacd is APPROVED. You have not set fedora-review to +. Many thanks! --Simone
Fixed.
Git done (by process-git-requests).