[Bug 1305091] Review Request: statsite - A C implementation of statsd

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1305091



--- Comment #2 from Denis Fateyev <denis@xxxxxxxxxxx> ---
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}

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]