Bug 1305091 - Review Request: statsite - A C implementation of statsd
Review Request: statsite - A C implementation of statsd
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Denis Fateyev
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2016-02-05 10:08 EST by Piotr Popieluch
Modified: 2016-05-27 17:03 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-05-27 17:03:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Piotr Popieluch 2016-02-05 10:08:50 EST
Spec URL: https://piotrp.fedorapeople.org/statsite.spec
SRPM URL: https://piotrp.fedorapeople.org/statsite-0.7.1-2.fc23.src.rpm
Description: 
Statsite is a metrics aggregation server. Statsite is based heavily on Etsy's
StatsD https://github.com/etsy/statsd, and is wire compatible.
Fedora Account System Username: piotrp
Comment 1 Piotr Popieluch 2016-02-09 07:42:36 EST
I just noticed that upstream bundled some libraries. Trying to unbundle it.
Comment 2 Denis Fateyev 2016-02-09 08:18:21 EST
Also, there are some points before review:

1) The `%if 0%{?rhel} && 0%{?rhel} <= 6` condition is used multiple times.
I personally prefer using something like that:

  %if 0%{?rhel} >= 7
  %global _with_systemd	 1
  %endif
  %if 0%{?fedora}
  %global _with_systemd	 1
  %endif
  ...
  %post
  %if 0%{?_with_systemd}
  %systemd_post %{name}.service
  %else
  /sbin/chkconfig --add %{name}
  %endif

(In other words, one global instead of multiple conditions). It looks more gracefully but sure, your option is also applicable;

2) Please wrap all el5-specific stuff into conditionals. Like that:
  %{?el5:BuildRoot:	%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)}
  ...
  %install
  %if 0%{?el5}
  rm -rf %{buildroot}
  %endif

3) Please add `%clean` section needed for el5.
  %if 0%{?el5}
  %clean
  rm -rf %{buildroot}
  %endif

4) Add all BRs (coreutils, gcc, make) required since there is no package exception list anymore;

5) This also can be replaced with single `install`:
  mkdir -p %{buildroot}%{_libexecdir}/%{name}
  cp -pr sinks %{buildroot}%{_libexecdir}/%{name}
Comment 3 Piotr Popieluch 2016-02-09 08:24:43 EST
Thank you for review. 

Unbundling will take some time as some of the deps are not in Fedora yet, and the source needs to be patched.

1,2,3) Agree, will implement

4) Aren't these part of the buildroot, thus not needed?

5) tried this initially but couldn't get it working because of the subdirs in sinks. Will check the man pages again, maybe I just missed some option.
Comment 4 Denis Fateyev 2016-02-09 08:32:40 EST
(In reply to Piotr Popieluch from comment #3)
> 4) Aren't these part of the buildroot, thus not needed?

They usually do, but it can be changed in the future.
Some details https://fedorahosted.org/fpc/ticket/497#comment:20

Please note the changes at https://fedoraproject.org/w/index.php?title=Packaging%3AGuidelines&diff=413629&oldid=409506
And the current version is: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_2

6) As for the Source0, a simplified version is also OK:
  Source0: https://github.com/armon/statsite/archive/v%{version}.tar.gz
But sure, if you feel that you need that "%{name}-%{version}" tail, it can be kept for sure.
Comment 5 Denis Fateyev 2016-02-09 11:48:25 EST
(In reply to Piotr Popieluch from comment #3)
> 5) tried this initially but couldn't get it working because of the subdirs
> in sinks. Will check the man pages again, maybe I just missed some option.
I think you're right. Didn't come up with a logical solution, so just keep the existing one. As for bundled files, let me know when I can proceed with the review.
Comment 6 Denis Fateyev 2016-02-23 12:04:19 EST
Any progress with it?
Comment 7 Piotr Popieluch 2016-02-23 14:05:51 EST
I'm sorry for the delay. I'm having some issues with unbundling, the application keeps coredumping on libev. Upstream master branch switched from libev to the redis event library, will focus on upstream/master. I will try to get some time for this later this week.
Comment 8 Piotr Popieluch 2016-05-27 17:03:31 EDT
Sorry for long delay, I didn't manage to get the libraries unbundled and lost interest in packaging this. closing

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