Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1305091 - Review Request: statsite - A C implementation of statsd
Summary: Review Request: statsite - A C implementation of statsd
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Denis Fateyev
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2016-02-05 15:08 UTC by Piotr Popieluch
Modified: 2016-05-27 21:03 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-05-27 21:03:31 UTC


Attachments (Terms of Use)

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


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