Bug 1305091

Summary: Review Request: statsite - A C implementation of statsd
Product: [Fedora] Fedora Reporter: Piotr Popieluch <piotr1212>
Component: Package ReviewAssignee: Denis Fateyev <denis>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: denis, package-review, piotr1212
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-05-27 21:03:31 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 201449    

Description Piotr Popieluch 2016-02-05 15:08:50 UTC
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 12:42:36 UTC
I just noticed that upstream bundled some libraries. Trying to unbundle it.

Comment 2 Denis Fateyev 2016-02-09 13:18:21 UTC
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 13:24:43 UTC
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 13:32:40 UTC
(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 16:48:25 UTC
(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 17:04:19 UTC
Any progress with it?

Comment 7 Piotr Popieluch 2016-02-23 19:05:51 UTC
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 21:03:31 UTC
Sorry for long delay, I didn't manage to get the libraries unbundled and lost interest in packaging this. closing