Bug 481224 - Review Request: rabbitmq-server - An AMQP server written in Erlang
Review Request: rabbitmq-server - An AMQP server written in Erlang
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-22 16:11 EST by Hubert Plociniczak
Modified: 2013-10-23 14:20 EDT (History)
5 users (show)

See Also:
Fixed In Version: 1.5.5-2.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-02 10:15:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Hubert Plociniczak 2009-01-22 16:11:34 EST
Spec URL: http://www.lshift.net/~hubert/rabbitmq-server.spec
SRPM URL: http://www.lshift.net/~hubert/rabbitmq-server-1.5.1-1.src.rpm
Description:
RabbitMQ is an implementation of AMQP, the emerging standard for high
performance enterprise messaging. The RabbitMQ server is a robust and
scalable implementation of an AMQP broker.

I am also looking for a sponsor.


rpmlint v0.85 returns following errors and warnings (which are not fixed on purpose):
rabbitmq-server.i386: E: no-binary
rabbitmq-server.i386: E: only-non-binary-in-usr-lib
rabbitmq-server.i386: W: dangerous-command-in-%post chown
rabbitmq-server.i386: W: service-default-enabled /etc/rc.d/init.d/rabbitmq-server
rabbitmq-server.i386: W: service-default-enabled /etc/rc.d/init.d/rabbitmq-server

The first two are because rpmlint doesn't properly recognize Erlang binary *.beam files and they need to be placed in the standard path (for example for i386 it is "/usr/lib/erlang/lib/").
The warnings about chown and service levels are also incorrect in this case, and are set on purpose.
Comment 1 Peter Lemenkov 2009-01-23 06:16:35 EST
Nice addition to Fedora/EPEL repository. If nobody will volunteer to review it, then I'll pick it up (probably diring next week or so).
Comment 2 Peter Lemenkov 2009-01-27 10:18:30 EST
Ok, I'll review it.
Comment 3 Peter Lemenkov 2009-02-02 10:39:29 EST
Remarks:

* According to book of Black Magic, called "Packaging Guidelines", you shouldn't use tags "Vendor" and "Packager". I suggest you to remove them both.

https://fedoraproject.org/wiki/Packaging/Guidelines#Tags

* Please, drop condition which checks for debian. 

* Add "%define debug_package %{nil}" on top of your spec to disallow creating useless debuginfo sub-package (see http://cvs.fedoraproject.org/viewvc/rpms/erlang-pgsql/devel/erlang-pgsql.spec?view=markup as an example).

* No need to explicitly define directory to unpack to in %prep section. I mean that "%setup" (or, better, "%setup -q") would be enough.

* You're using make w/o %{?_smp_mflags} switch. Does rabbitmq-server support parallel make? If not, then you should add note (see %build section it this spec-file - http://cvs.fedoraproject.org/viewvc/rpms/ejabberd/devel/ejabberd.spec?view=markup )

I'll suggest some improvements later.
Comment 4 Peter Lemenkov 2009-02-03 05:55:14 EST
* In some cases you don't need to create directories before installing files to them, because 'install' utility has special switch '-D'. E.g. instead of

mkdir -p %{buildroot}%{_initrddir}
install -m 0755 %SOURCE1 %{buildroot}%{_initrddir}/rabbitmq-server

you may write

install -D -m 0755 %SOURCE1 %{buildroot}%{_initrddir}/rabbitmq-server

* Invoke 'install' with '-p' switch while installing files (in order to preserve timestamps). 

* This line should be removed (looks like leftover)

chmod 0755 %{buildroot}%{_initrddir}/rabbitmq-server

* you should consider using 

rm %{_maindir}/{LICENSE,LICENSE-MPL-RabbitMQ,INSTALL}

instead of

rm %{_maindir}/LICENSE %{_maindir}/LICENSE-MPL-RabbitMQ %{_maindir}/INSTALL

* regarding init-script. It should not be started by default. So, please, fix chkconfig's header from

#chkconfig: 2345 80 05

to 

#chkconfig: - 80 05

You should fix init-header also.

* It's a generally good idea to move some commandline switches from start section of initscript to /etc/sysconfig/%{name} 

See this, for example:

http://cvs.fedoraproject.org/viewvc/rpms/ejabberd/devel/ejabberd.init?view=markup
http://cvs.fedoraproject.org/viewvc/rpms/ejabberd/devel/ejabberd.sysconfig?view=markup
Comment 5 Hubert Plociniczak 2009-02-04 08:03:17 EST
Thanks for the review Peter. All of those seem reasonable and I will fix them promptly. But I've got one question regarding init-script run levels.

I know that for example ejabberd also isn't started by default but it seems somehow natural for rabbit users that after reboot/crash the server is up again. Possibly run levels 2345 is too much and 3 5 would be definitely enough.
Can you elaborate on the rationale against starting it by default?
http://fedoraproject.org/wiki/Packaging/SysVInitScript doesn't seem to be strictly against it.
Comment 6 Ruben Kerkhof 2009-02-07 04:19:57 EST
The choice to start a service by default is left to the user, to do that he has to run 'chkconfig rabbitmq on' after installing the rpm. The rationale behind it is that after installation no unwanted services are started without the users knowledge, and him having the time to secure the ports with firewall rules etc. Since RabbitMQ listens on the network it shouldn't be started by default.

Does that make sense?

Ruben
Comment 7 Hubert Plociniczak 2009-02-10 17:28:51 EST
Yeap, seems reasonable. I will fix that.
Comment 8 Peter Lemenkov 2009-02-16 10:38:06 EST
Ping, Hubert :)
Comment 9 Hubert Plociniczak 2009-02-17 18:51:13 EST
There is an upcoming minor release 1.5.2 which will also include changes to the rpm packaging that you mentioned. This should be really soon. Sorry for the delay!
Comment 10 Hubert Plociniczak 2009-02-25 06:10:37 EST
Spec URL: http://www.lshift.net/~hubert/rabbitmq-server.spec
SRPM URL: http://www.lshift.net/~hubert/rabbitmq-server-1.5.3-1.src.rpm

These include fixes for all the problems you mentioned apart from that:

* you should consider using 

rm %{_maindir}/{LICENSE,LICENSE-MPL-RabbitMQ,INSTALL}

instead of

rm %{_maindir}/LICENSE %{_maindir}/LICENSE-MPL-RabbitMQ %{_maindir}/INSTALL

There reason for this is that this pattern is available for some newer version of rpmbuild, but we also tend to make releases on machines that have version 4.4.2.1 installed and does not support it.
It is only a shortcut, but works ok without it. I hope you are ok with that?
I also included few other fixes, you will probably see them in the spec etc.
Comment 11 Peter Lemenkov 2009-03-05 14:21:38 EST
Still Here, Hubert. Sorry for the delay.
I'll post my future comments asap.
Comment 12 Peter Lemenkov 2009-03-07 09:48:35 EST
The only issue I found so far  is the mystic typo in sed oneliner at lines 39,40. 

Actually, it's not a typo, but rather issue because of silent changing files in ~/rpmbuild/SOURCE. You should NOT change files there (you're using -i sed switch). Instead of it, copy necessary files to builddir and change them here.

Other things looks sane.
Comment 13 Peter Lemenkov 2009-03-16 07:45:14 EDT
Ping, again.
Comment 14 Hubert Plociniczak 2009-03-17 17:59:22 EDT
Sorry about that, I was quite busy lately. I will try to fix this small thing shortly. Thanks!
Comment 15 Hubert Plociniczak 2009-04-11 17:04:15 EDT
Spec URL: http://dev.lshift.net/hubert/rabbitmq-server.spec
SRPM URL: http://dev.lshift.net/hubert/rabbitmq-server-1.5.4-1.src.rpm

This is the new minor release done just a couple of days ago.
It contains fix for the sed problem.
Comment 16 Peter Lemenkov 2009-04-30 05:39:48 EDT
Sorry for the delay.

OK, the spec-file is legible, but I overlooked the naive approach to security with rabbit_wrapper. I think that this trick is useless, so I advise you just to remove all %{_rabbit_wrapper}-related stuff.
Comment 17 Hubert Plociniczak 2009-05-04 12:22:52 EDT
One of the point of having wrappers is to ensure that the server is started using proper permissions and running as a proper user. We tend to get a lot of bug reports related to this stuff, which aren't really bugs, because users simply forget about it. So I would rather leave as it is.
Comment 18 Peter Lemenkov 2009-05-04 13:40:42 EDT
Understood (however, I still don't like this silly solution).

* Ok, it builds fine in Koji for F-11:
>> http://koji.fedoraproject.org/koji/taskinfo?taskID=1335580
Unfortunately, it failed to build in EL-5:
>> http://koji.fedoraproject.org/koji/taskinfo?taskID=1335591
If you decided to add EPEl branch for rabbitmq, then this needs fixing.

* rpmling is not silent:

[petro@Sulaco ~]$ rpmlint Desktop/rabbitmq-server-1.5.4-1.ppc.rpm 
rabbitmq-server.ppc: E: no-binary
rabbitmq-server.ppc: W: only-non-binary-in-usr-lib
rabbitmq-server.ppc: W: non-standard-uid /var/lib/rabbitmq rabbitmq
rabbitmq-server.ppc: W: non-standard-gid /var/lib/rabbitmq rabbitmq
rabbitmq-server.ppc: E: non-standard-dir-perm /var/lib/rabbitmq 0750
rabbitmq-server.ppc: W: non-standard-uid /var/log/rabbitmq rabbitmq
rabbitmq-server.ppc: W: non-standard-gid /var/log/rabbitmq rabbitmq
rabbitmq-server.ppc: E: non-standard-dir-perm /var/log/rabbitmq 0750
rabbitmq-server.ppc: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/rabbitmq-server
1 packages and 0 specfiles checked; 3 errors, 6 warnings.
[petro@Sulaco ~]$

All these messages, except missing-lsb-keyword may be ignored. Message "missing-lsb-keyword" should be suppressed by adding proper LSB keyword to init-script (at least with empty value).

BTW are you still searching for sponsor? I cannot find you in "sponsorship queue". What's your Fedora account name?
Comment 19 Hubert Plociniczak 2009-05-05 15:59:12 EDT
(In reply to comment #18)
> Understood (however, I still don't like this silly solution).

In future we will probably remove the batch scripts anyway and use something more sensible.

> * Ok, it builds fine in Koji for F-11:
> >> http://koji.fedoraproject.org/koji/taskinfo?taskID=1335580
> Unfortunately, it failed to build in EL-5:
> >> http://koji.fedoraproject.org/koji/taskinfo?taskID=1335591
> If you decided to add EPEl branch for rabbitmq, then this needs fixing.

I will check that now.


> * rpmling is not silent:
> rabbitmq-server.ppc: W: missing-lsb-keyword Default-Stop in
> /etc/rc.d/init.d/rabbitmq-server
> 1 packages and 0 specfiles checked; 3 errors, 6 warnings.
> [petro@Sulaco ~]$
> 

Apparently my version of rpmlint didn't catch this one. I will fix this.

> BTW are you still searching for sponsor? I cannot find you in "sponsorship
> queue". What's your Fedora account name?  

hubert

Thanks!
Comment 20 Peter Lemenkov 2009-05-06 01:15:21 EDT
Just found another one easy-to-fix issue - no need to ship INSTALL file in %doc. It's useless. Please remove it.

REVIEW:

- rpmlint is not silent - see above. Easy to fix, anyway.
+ The package is named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license.
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+/- The spec file for the package MUST be legible (except the silly trick with rabbitmq-script-wrapper - I advice you to add some explanations in spec-file, however it's not a blocker).

- The sources, used to build the package, MUST match the upstream source, as provided in the spec URL.

[petro@Sulaco SOURCES]$ md5sum rabbitmq-server-1.5.4.tar.gz*
dab74bc1a3051cfc94a11abeabb8b0c6  rabbitmq-server-1.5.4.tar.gz
9d43f979d533df743ca7f6050f142040  rabbitmq-server-1.5.4.tar.gz.1
[petro@Sulaco SOURCES]$

This is a blocker. Please fix it.

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji buildlog in the comments above.
+ All build dependencies are listed in BuildRequires.
+ A package owns all directories that it creates.
+ No files, listed more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code, or permissible content.
+ No large documentation files.
+ Everything, a package includes as %doc, does not affect the runtime of the application.

- Header files must be in a -devel package. Consider creating devel-subpackage for %{_rabbit_erllibdir}/include. Please, note, that in the vast majority of cases, devel packages must require the base package (using a fully versioned dependency: Requires: %{name} = %{version}-%{release}). 

+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in rpm packages are valid UTF-8.

Ok, let's summarize things. Required to fix:

* Add proper source-file to srpm (md5 must match)
* Move headers into devel sub-package
* Fix rpmlint warning about missing-lsb-keyword
* Exclude INSTALL file from %doc.

Should fix (not a blocker - just to your consideration)

* Add comments, explaining usage of rabbitmq-script-wrapper.
* Discover the issue with build failure on EPEL (I'm sure of importance EPEL branch of rabbitmq-server).

These are the last obstacles on our road :)
Comment 21 David Wragg 2009-05-13 11:52:20 EDT
Hubert is busy with his studies at the moment, so I'm taking care of the remaining issues raised by Peter.

I'm almost done addressing the blockers raised, and the EPEL build failure.  But after discussion about the requirement for a -devel package with the rabbitmq development team, we are reluctant to split the package in this way.  While the rabbitmq server can run without the .hrl files, they have uses that are not restricted to development activities:  They are useful to allow interaction with the server at the erlang command line, for administration and monitoring.  So we don't believe that they fall into the same category as the header files of a library, as described in the "Devel packages" section of the packaging guidelines.

Peter, do you feel that this context be taken into account?
Comment 22 Peter Lemenkov 2009-05-17 07:29:29 EDT
(In reply to comment #21)

> But after discussion about the requirement for a -devel package with the
> rabbitmq development team, we are reluctant to split the package in this way. 
> While the rabbitmq server can run without the .hrl files, they have uses that
> are not restricted to development activities:  They are useful to allow
> interaction with the server at the erlang command line, for administration and
> monitoring.  So we don't believe that they fall into the same category as the
> header files of a library, as described in the "Devel packages" section of the
> packaging guidelines.
  
The problem is that we (I mean people, who interested in getting more erlang software into Fedora/EPEL) still haven't a rules/policy for packaging erlang applications and libraries. Among (still very few) erlang-related packages in Fedora/EPEL, some packaged splitted into main and *-devel subpackage, while others - not (erlang itself).

After some thinking about this issue, I finally agreed with you - we should bundle hrl-files with main package, and this mine request should be ignored.
Comment 23 Ruben Kerkhof 2009-05-19 10:25:09 EDT
(In reply to comment #21)

Hi David,

Please note that I've sponsored Hubert, but if you are going to continue his work you'll have to apply for sponsorship as well.

(In reply to comment #22)

Peter, maybe we can start an Erlang SIG or something like that? There are a lot of erlang packages which would be nice to have in Fedora, and it would be easier to package them up if this is a joined effort. I'd like to tackle the rabbitmq-xmpp bridge next for instance.
Comment 24 David Wragg 2009-05-19 10:49:33 EDT
(In reply to comment #23)
> Hi David,
> 
> Please note that I've sponsored Hubert, but if you are going to continue his
> work you'll have to apply for sponsorship as well.

Hi Ruben,

Thanks for the note.  I was just keeping things moving while Hubert was busy with other commitments.  Hubert is still around, and I expect he will continue the submission process, so my involvement is probably more or less over.  If that situation changes, I will apply for sponsorship.

David
Comment 25 Hubert Plociniczak 2009-05-19 11:57:24 EDT
(In reply to comment #24)
Yes, I am back. Thanks for help, David.
We should have a minor release pretty soon with all the changes included, so I will post it here as soon as it is available.
Comment 26 Peter Lemenkov 2009-05-21 04:42:08 EDT
(In reply to comment #23)
 
> Peter, maybe we can start an Erlang SIG or something like that? 

Already did :)

https://fedoraproject.org/wiki/Erlang

However, there is not much lavuable info on that page (no guidelines yet). But feel free to modify it.

> There are a lot of erlang packages which would be nice to have in Fedora, 
> and it would be easier to package them up if this is a joined effort. 

Agree.
Comment 27 Peter Lemenkov 2009-05-21 04:42:45 EDT
(In reply to comment #26)

> However, there is not much lavuable info on that page (no guidelines yet). 

s/lavuable/valuable/
Comment 28 Hubert Plociniczak 2009-05-21 06:46:55 EDT
Spec URL: http://dev.lshift.net/hubert/rabbitmq-server.spec
SRPM URL: http://dev.lshift.net/hubert/rabbitmq-server-1.5.5-1.src.rpm

This release contains fixes for the problems mentioned above.
Tested on koji for F-11 and EL-5 and works fine now.
rpmlint is not silent but we fixed 'missing-lsb-keyword' error and the rest can be ignored.
Comment 29 Peter Lemenkov 2009-05-21 07:01:54 EDT
Ok

+ md5 is correct

[petro@Sulaco SOURCES]$ md5sum rabbitmq-server-1.5.5.tar.gz*
1dceb98bb57cd6acef90f58e96a7fce4  rabbitmq-server-1.5.5.tar.gz
1dceb98bb57cd6acef90f58e96a7fce4  rabbitmq-server-1.5.5.tar.gz.1
[petro@Sulaco SOURCES]$

+ rpmlint is not silent but these messages already explained (see https://bugzilla.redhat.com/show_bug.cgi?id=481224#c18 )

[petro@Sulaco SPECS]$ rpmlint ../RPMS/ppc/rabbitmq-server-1.5.5-1.ppc.rpm 
rabbitmq-server.ppc: E: no-binary
rabbitmq-server.ppc: W: only-non-binary-in-usr-lib
rabbitmq-server.ppc: W: non-standard-uid /var/lib/rabbitmq rabbitmq
rabbitmq-server.ppc: W: non-standard-gid /var/lib/rabbitmq rabbitmq
rabbitmq-server.ppc: E: non-standard-dir-perm /var/lib/rabbitmq 0750
rabbitmq-server.ppc: W: non-standard-uid /var/log/rabbitmq rabbitmq
rabbitmq-server.ppc: W: non-standard-gid /var/log/rabbitmq rabbitmq
rabbitmq-server.ppc: E: non-standard-dir-perm /var/log/rabbitmq 0750
1 packages and 0 specfiles checked; 3 errors, 5 warnings.
[petro@Sulaco SPECS]$

Ok, this package is


APPROVED.
Comment 30 Hubert Plociniczak 2009-05-21 07:46:57 EDT
New Package CVS Request
=======================
Package Name: rabbitmq-server
Short Description: An AMQP server written in Erlang
Owners: hubert
Branches: F-9 F-10 F-11 EL-5
InitialCC:
Comment 31 Kevin Fenzi 2009-05-21 19:45:13 EDT
cvs done.
Comment 32 Fedora Update System 2009-05-26 13:43:32 EDT
rabbitmq-server-1.5.5-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/rabbitmq-server-1.5.5-2.fc10
Comment 33 Fedora Update System 2009-05-26 13:43:40 EDT
rabbitmq-server-1.5.5-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/rabbitmq-server-1.5.5-2.fc11
Comment 34 Fedora Update System 2009-05-26 13:43:46 EDT
rabbitmq-server-1.5.5-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/rabbitmq-server-1.5.5-2.fc9
Comment 35 Fedora Update System 2009-05-28 04:02:16 EDT
rabbitmq-server-1.5.5-2.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update rabbitmq-server'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-5563
Comment 36 Fedora Update System 2009-05-28 04:05:19 EDT
rabbitmq-server-1.5.5-2.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update rabbitmq-server'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-5581
Comment 37 Fedora Update System 2009-05-28 04:07:39 EDT
rabbitmq-server-1.5.5-2.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update rabbitmq-server'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2009-5601
Comment 38 Fedora Update System 2009-06-02 10:15:27 EDT
rabbitmq-server-1.5.5-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 39 Fedora Update System 2009-06-02 10:17:14 EDT
rabbitmq-server-1.5.5-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 40 Fedora Update System 2009-06-02 10:32:03 EDT
rabbitmq-server-1.5.5-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 41 Hubert Plociniczak 2009-06-02 11:15:42 EDT
Thanks for help with the review, Peter!
Comment 42 Peter Lemenkov 2013-10-23 14:03:20 EDT
Package Change Request
======================
Package Name: rabbitmq-server
InitialCC: erlang-sig
Comment 43 Gwyn Ciesla 2013-10-23 14:20:14 EDT
Done.

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