Bug 1060386 - (pandorafms-agent) Review Request: pandorafms-agent - Pandora FMS Linux agent.
Review Request: pandorafms-agent - Pandora FMS Linux agent.
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
http://pandorafms.com
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2014-01-31 21:51 EST by Sancho Lerena
Modified: 2016-02-08 08:49 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-02-08 08:49:23 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Sancho Lerena 2014-01-31 21:51:07 EST
Spec URL: http://artica.es/f/index.php/files/get/hejpLEqExP/pandora-agent.redhat-.spec
SRPM URL: http://artica.es/f/index.php/files/get/0n2Pj1E05v/pandorafms-agent-unix-5.0sp3-140201-.src-.rpm
Description: Pandora FMS agent for unix. Pandora FMS is an OpenSource full-featured monitoring software.
Fedora Account System Username: slerena

Hello, this is my first package request on Fedora and I looking for a sponsor. I'm the project leader of Pandora FMS, an opensource monitoring project, started in 2003 and with almost 20 releases (http://pandorafms.com). We have packages compatibles for major distributions and our official supported platform is CentOS. I would like to include Pandora FMS and help to maintain in EPEL repository. 

This is one of the packages which composes pandora (there are three in total), this one is the agent for monitoring, running on each server monitored. It's based on perl, and I would like to submit also the management console (PHP) and the server (Perl), but try first with the agent, because is the easiest one.

I build the package with koji successful:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6479040

Thanks !
Comment 1 Christopher Meng 2014-01-31 23:07:24 EST
Ugly bad name.


pandora-agent.redhat-.spec

Funny.
Comment 2 Sancho Lerena 2014-02-01 08:43:30 EST
Sorry, updated URL (same content, fixed file names):

Spec URL: http://code.pandorafms.com/files/pandora_agent.redhat.spec
SRPM URL: http://code.pandorafms.com/files/pandorafms_agent_unix-5.0SP3-140201.src.rpm
Comment 4 Sancho Lerena 2014-02-01 13:15:33 EST
I've read again the guidelines and fixed the bad file naming. I've regenerated the SRPM and the spec, also modifying release, version, added a changelog section and removed prerequisite token.

Files are now in:

http://code.pandorafms.com/files/pandorafms-agent-5.0-140201.sp3.src.rpm
http://code.pandorafms.com/files/pandorafms-agent.spec

About the fedora-review process, I think that should be done by a reviewer, right?. I've tried and get this error:

fedora-review -b 1060386
ERROR: 'No mock group - mock not installed or mock not in effectivegroups. Try running  "newgrp" or logging out from all your local sessions and logging back in. Or disable test using REVIEW_NO_MOCKGROUP_CHECK, see manpage' (logs in /root/.cache/fedora-review.log)
Comment 5 Alec Leamas 2014-02-19 04:04:23 EST
Hi, and welcome!

Now, since this is your first package here are two things: you need to improve your package, and to get sponsored. As for improving your package some points out of the top of my head:
- The installation is quite messy. This seems to be because you have to do it more or less from scratch in the spec file. Applications like this normally has an installer script which can do something like "make install DESTDIR=$RPM_BUILD_ROOT". It would probably be better to patch your already existing installer to respect a DESTDIR argument or environment variable (if it doesn't already support this) and use it in the %install section. This would clean up the spec considerably, and use already proven code to install.
- You don't really understand how rpm works e. g., when upgrading and tries to duplicate it's behaviour. This will break expectations. In short, trust how rpm upgrades files, also in /etc. It's enough if you mark them as %config files.
- As an example, the long %post section could probably be (almost?) skipped. All files and dirs should be installed in %install, and enabling a service during installation is not allowed.
- You can certainly run fedora-review yourself as a preparation for the review. The manpage describes how to add the required group to your user (don't paste the results into the ticket, though - that's the reviewer's task).

But to me, what looks like the hard part is to get sponsored (no, I'm not a sponsor myself). In order to get sponsored you need to look into [1]. In particular, you need to look into other reviews and make remarks. By looking at other reviews (and remarks from others!) you will get a better understanding of both the guidelines and the process. For now, I think it might make sense to focus on this; it will eventually make you update your package in many ways beyond my remarks.

As a starter, you can find other open review requests at [2]

[1] https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
[2] http://fedoraproject.org/PackageReviewStatus/NEW.html
Comment 6 Alec Leamas 2014-02-19 04:17:31 EST
Ah, as for fedora-review message: it says you need to install the mock package (# yum install mock). Nothing else, my bad.
Comment 7 Christopher Meng 2014-02-20 04:12:07 EST
Ugly spec.

1. Remove this:

#
# spec file for package pandorafms-agent
#
# Pandora FMS Linux Agent
# Copyright (c) 2014 Sancho Lerena
# Licensed under GPL2 terms.
# Please send bugfixes or comments to slerena@xxx

Your email will be leaked to public for spammer.

2. %define name        pandorafms-agent
%define version     5.0
%define release     140201.sp3

Please use proper tag, no need to define a macro to finish that.

3. Remove these:
Vendor:             ArticaST <http://www.artica.es>
Group:              System/Monitoring
Packager:           Sancho Lerena <slerena@xxxxxxx>
Prefix:             /usr/share
BuildRoot:          %{_tmppath}/%{name}-%{version}-buildroot

For this one /usr/share, please

rpm -E %{_datadir}

4. rm -rf $RPM_BUILD_ROOT is not needed in %prep and %install

5. These are not done by command, should be done by RPM:

# Checking old config file (if exists)
if [ -f /etc/pandora/pandora_agent.conf ] ; then
	mv /etc/pandora/pandora_agent.conf /etc/pandora/pandora_agent.conf.backup
fi

cp -aRf $RPM_BUILD_ROOT%{prefix}/pandora_agent/Linux/pandora_agent.conf $RPM_BUILD_ROOT/usr/share/pandora_agent/pandora_agent.conf.rpmnew

if [ -f $RPM_BUILD_ROOT%{prefix}/pandora_agent/pandora_agent.spec ] ; then
	rm $RPM_BUILD_ROOT%{prefix}/pandora_agent/pandora_agent.spec
fi

So remove them.

6. No %clean section please.

7. %pre is not fine:

https://fedoraproject.org/wiki/Packaging:UsersAndGroups#Dynamic_allocation

8. Please learn how to write changelog:

* Sat Feb 01 2014 - slerena@xxxxx
- First version, after re-re-re-reading the fedora contributor guidelines :)

Take a look at existing one:

http://pkgs.fedoraproject.org/cgit/nagios.git/tree/nagios.spec

God, totally a mess, please clean above at first, then step forward. I can help you, but don't be hasty.
Comment 8 Michael Schwendt 2014-02-20 06:21:03 EST
> Prefix:             /usr/share

Defining %{prefix} via a Prefix tag would mark the package as being relocatable. That's important to know, and the following applies:

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


Currently, the package contents are not relocatable, so overriding %prefix with /usr/share is just a bad idea. Instead, use existing macros that inherit from eachother, such as %_bindir, %_datadir, %_mandir.
https://fedoraproject.org/wiki/Packaging:Guidelines#Macros

[...]

> About the fedora-review process, I think that should be done by a reviewer,
> right?. 

Why would you not perform a self-review of your own package with the help of tools like "rpmlint -i" (mandatory) and "fedora-review"? ;-)  How do you decide whether you believe your package would pass review? You need a reviewer for the approval of the package, but not for re-writing your spec file. If you haven't installed/used Mock before, doing that would be good exercise.

Beyond that, all package submitters are expected to be able to know the ReviewGuidelines and also do a few reviews of packages in the queue.

  https://fedoraproject.org/wiki/Category:Package_Maintainers
  https://fedoraproject.org/wiki/Package_Review_Process
  http://fedoraproject.org/PackageReviewStatus/


> http://code.pandorafms.com/files/pandorafms-agent.spec

I've examined the diff against the first version, and you haven't changed much.

Please start with running "rpmlint -i" on the src.rpm *and* all built rpms *at once*. Get familiar with rpmlint errors/warnings. Feel free to ignore obvious false positives in the report, but fix anything else. Preferably add a comment here about whether/when you think what rpmlint reports is correct or incorrect.

The spec file does a lot of questionable/strange/unusual things without any comments in the spec file. Some of it is not covered by the packaging guidelines, because nobody would do such things. If you added comments/explanations in your %install, %preun and %post sections, for example, a reviewer and/or sponsor could understand _why_ you think you need to do the stuff you do in those sections. As a start, please try to explain what you do in %install, %preun and %post sections and why you do it in those sections.
Comment 9 Sancho Lerena 2014-02-21 10:15:56 EST
Wow. Thanks for the information, I will have a busy weekend working on all of these :)
Comment 10 Sancho Lerena 2014-02-23 20:17:35 EST
Hello again,

I tried to focus on rewrite the SPEC based on some suggestion and example (nagios) spec. Thanks for the help.

The updated spec is at the same URL (overwriting the old one):

http://code.pandorafms.com/files/pandorafms-agent.spec

rpmlint passed ok on spec and SRPM. On binary RPM get lots of E/W and some of them and frustrating :(

I have some problems (understanding the way it behaves) about the files included in a directory /plugins which I copy manually (with CP). It works ok, but rpmlink on binary RPM got lots of errors :(

Any advice here will be very appreciated. 

Thanks.
Comment 11 Christopher Meng 2014-02-23 20:52:33 EST
OK.

Second stage(dont be panic, a lot still):

0. Why don't you use tarball at here: 

http://sourceforge.net/projects/pandora/files/Pandora%20FMS%205.0/FinalSP3/Tarball/pandorafms_agent_unix-5.0SP3.tar.gz

as source?

1. Vendor: Artica <http://www.artica.es>

Remove this.

2. Please remove the lines, don't comment them.

3. No need to add these IMO: Requires(pre): /bin/sed /bin/grep 

4. Requires(preun): initscripts, chkconfig
Requires(post): initscripts, chkconfig
Requires(postun): initscripts

I'm sorry, please learn systemd, and write a service file:

https://fedoraproject.org/wiki/Packaging:Systemd

4. rm -rf %{buildroot} in %install section is not needed, delete it.

5. Please perserve the timestamp with install -p option:

6. %description is too poor, you are the upstream right? Why not improve it?

7. You didn't read comment 7 carefully, please remove %clean section.

8. changelog release field syntax incorrect.

Please rpmdev-bumpspec to bump the spec and see what will happen, then fix it.

9. Please learn and replace macros: 

https://fedoraproject.org/wiki/Packaging:RPMMacros
Comment 12 Christopher Meng 2014-02-23 20:57:23 EST
For issue 8, an extra question:

Release: 140223.sp3%{?dist}

Fedora packages seldom have release number up to 10^5, you'd better start from 1, but rebuild may destroy what you expect and may cause upgrade path issues, therefore please take a look at here for package has non-numeric version:

https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning
Comment 13 Michael Schwendt 2014-02-24 20:03:14 EST
Running "fedora-review -b 1060386" fails. Please keep the "Spec URL:" and "SRPM URL:" lines up-to-date.

[...]

Which rpmlint warnings/error do you find frustrating?

Try "rpmlint -i …" on the built rpm. The -i adds helpful output. If you get too many W/E, you can also query rpmlint for its help, e.g.

  $ rpmlint -I service-default-enabled
  service-default-enabled:
  The service is enabled by default after "chkconfig --add"; for security
  reasons, most services should not be. Use "-" as the default runlevel in the
  init script's "chkconfig:" line and/or remove the "Default-Start:" LSB keyword
  to fix this if appropriate for this service.


  $ rpmlint -I conffile-without-noreplace-flag
  conffile-without-noreplace-flag:
  A configuration file is stored in your package without the noreplace flag. A
  way to resolve this is to put the following in your SPEC file:
  %config(noreplace) /etc/your_config_file_here

[...]

> pandorafms-agent.noarch: W: dangerous-command-in-%preun userdel

https://fedoraproject.org/wiki/Packaging:UsersAndGroups


> %post
> mkdir -p /var/spool/pandora/data_out

Why aren't these directories included in the package?


> %doc

An empty %doc does nothing.

> /usr/share/man/man1/pandora_agent.1.gz
> /usr/share/man/man1/tentacle_client.1.gz

$ rpm -E %_mandir
/usr/share/man

Typically, one uses wildcards and macros to include the files:

  %{_mandir}/man1/*.1*

Not specifying the trailing .gz is not a big issue, but allows for disabling/changing/customizing compressing of manual pages without breaking the spec file.
Comment 14 Miroslav Suchý 2015-07-21 11:03:26 EDT
Sancho, any progress here? Are you still interrested in this review?
Comment 15 Upstream Release Monitoring 2015-12-06 13:26:00 EST
pbrobinson's scratch build of linux-user-chroot?#b7afe5173cbd31b029b027b6f8a14baa5e6ce87a for epel7-archbootstrap and git://pkgs.fedoraproject.org/linux-user-chroot?#b7afe5173cbd31b029b027b6f8a14baa5e6ce87a failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12089939
Comment 16 Miroslav Suchý 2016-02-08 08:49:23 EST
No response. Closing as dead review. If you ever want to continue, please resubmit.

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