Bug 820561 - Review Request: guacd - Proxy daemon for Guacamole
Review Request: guacd - Proxy daemon for Guacamole
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Terje Røsten
Fedora Extras Quality Assurance
:
Depends On: 820542 820543 820544
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 07:51 EDT by Simone Caronni
Modified: 2012-06-11 09:27 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-11 09:27:00 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
terjeros: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Simone Caronni 2012-05-10 07:51:49 EDT
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/
Comment 1 Mads Kiilerich 2012-05-10 08:13:48 EDT
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.
Comment 2 Simone Caronni 2012-05-10 08:37:08 EDT
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
Comment 3 Mads Kiilerich 2012-05-10 08:44:03 EDT
(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. ;-)
Comment 4 Simone Caronni 2012-05-10 09:05:26 EDT
(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
Comment 5 Mads Kiilerich 2012-05-10 09:22:27 EDT
(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.
Comment 6 Simone Caronni 2012-05-10 09:46:22 EDT
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
Comment 7 Simone Caronni 2012-05-15 04:51:59 EDT
libguac on which this package is dependant on has been approved.

--Simone
Comment 8 Terje Røsten 2012-05-25 13:45:34 EDT
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.*
Comment 9 Simone Caronni 2012-05-28 08:36:37 EDT
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.
Comment 10 Terje Røsten 2012-05-28 13:59:17 EDT
This is not building in mock, missing systemd-units in buildreq?

RPM build errors:
    File must begin with "/": %{_unitdir}/guacd.service
Comment 11 Simone Caronni 2012-05-28 15:57:39 EDT
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
Comment 12 Simone Caronni 2012-05-28 16:01:27 EDT
Tested with koji and f16, it builds fine also on f16 without a BR on systemd-units.
Comment 13 Simone Caronni 2012-05-28 16:13:12 EDT
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
Comment 14 Terje Røsten 2012-05-28 17:23:20 EDT
==== 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
Comment 15 Simone Caronni 2012-05-28 17:32:52 EDT
(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
Comment 16 Terje Røsten 2012-05-28 17:34:55 EDT
Thanks for quick fixes.

 Package guacd is APPROVED.
Comment 17 Simone Caronni 2012-05-28 17:37:18 EDT
New Package SCM Request
=======================
Package Name: guacd
Short Description: Proxy daemon for Guacamole
Owners: slaanesh
Branches: f17 f16 el6
InitialCC:
Comment 18 Simone Caronni 2012-05-29 07:07:05 EDT
(In reply to comment #16)
> Thanks for quick fixes.
> 
>  Package guacd is APPROVED.

You have not set fedora-review to +.

Many thanks!
--Simone
Comment 19 Terje Røsten 2012-05-29 07:30:16 EDT
Fixed.
Comment 20 Gwyn Ciesla 2012-05-29 09:30:18 EDT
Git done (by process-git-requests).

Note You need to log in before you can comment on or make changes to this bug.