Bug 1302904 - Review Request: cacti - re-review
Review Request: cacti - re-review
Status: POST
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-28 18:16 EST by Morten Stevens
Modified: 2016-04-15 09:59 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Morten Stevens 2016-01-28 18:16:42 EST
Spec URL: https://mstevens.fedorapeople.org/cacti/cacti.spec
SRPM URL: https://mstevens.fedorapeople.org/cacti/cacti-0.8.8f-1.el7.noarch.rpm
Description: This is a re-review request for cacti. The package is based on the orphaned Fedora/EPEL package.
Fedora Account System Username: mstevens
Comment 1 Morten Stevens 2016-01-29 10:24:25 EST
New SRPM: https://mstevens.fedorapeople.org/cacti/cacti-0.8.8f-2.el7.src.rpm
New SPEC: https://mstevens.fedorapeople.org/cacti/cacti.spec

Changes:
Added 3 security patches to fix CVE-2015-8369, CVE-2015-8377 and CVE-2015-8604.
Comment 2 Zbigniew Jędrzejewski-Szmek 2016-04-04 14:02:37 EDT
Comments about the spec file:
- Group can be dropped
- %_pkgdocdir is now always defined, you can drop the compat define
- Use %autosetup
- Drop %clean
- Drop "rm -rf %{buildroot}"
- For heavens' sake, replace %{__mkdir} with mkdir, %{__install} with install, and so on.
- Use proper systemd scriptlets [https://fedoraproject.org/wiki/Packaging:Scriptlets#Scriptlets]
- Drop defattr
Comment 3 Morten Stevens 2016-04-12 12:47:15 EDT
Okay, please check:

Updated SRPM: https://mstevens.fedorapeople.org/cacti/cacti-0.8.8g-1.fc25.src.rpm
Updated SPEC: https://mstevens.fedorapeople.org/cacti/cacti.spec

All objections should be resolved. But I think we need %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}} to stay compatible with EPEL-7.
Comment 4 Zbigniew Jędrzejewski-Szmek 2016-04-12 16:52:10 EDT
So..., the same for %{__cp}, %{__mv}, %{__chmod} [See https://fedoraproject.org/wiki/Packaging:Guidelines#Macros].
You can just call useradd too, /usr/sbin is nowawadays in the $PATH even for normal users.

BR: systemd
%{systemd_requires}

Your package reloads httpd.service. This is OK, but then you shouldn't call %systemd_post. This is used to preset (apply default enablement) to a service. For httpd.service this should only be done by the package that installs it. OTOH, you also need to reload httpd.service after installing your package, i.e. in %post:

%post
if [ $1 -eq 1 ] ; then
        %systemd_postun_with_restart httpd.service
fi 

The spec file must specify licensing breakdown [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios].

Why do you need %patch0 if you have %autosetup? %autosetup should do patching automatically.

Shouldn't /var/log/cacti/cacti.log be %ghost?

%description should be wrapped to 80 columns.
Comment 5 Morten Stevens 2016-04-14 06:57:30 EDT
All done. Please check:

Updated SRPM: https://mstevens.fedorapeople.org/cacti/cacti-0.8.8g-1.el7.src.rpm
Updated SPEC: https://mstevens.fedorapeople.org/cacti/cacti.spec

> Why do you need %patch0 if you have %autosetup? %autosetup should do
> patching automatically.

I'm not sure, but the patch was not applied and we need to apply the patch before cp %{SOURCE4} %{SOURCE5} %{SOURCE6} include/js/jquery/themes/default/
Comment 6 Zbigniew Jędrzejewski-Szmek 2016-04-14 12:41:19 EDT
If possible, it would be better to use the system wide jquery package. If not possible, add Provides for bundled jquery [https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries].

Those should not be executable:
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_diagonals-thick_18_b81900_40x40.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_diagonals-thick_20_666666_40x40.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_flat_10_000000_40x100.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_glass_100_f6f6f6_1x400.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_glass_100_fdf5ce_1x400.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_glass_65_ffffff_1x400.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_gloss-wave_35_f6a828_500x100.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-icons_222222_256x240.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-icons_228ef1_256x240.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-icons_ef8c08_256x240.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-icons_ffd27a_256x240.png
-rwxr-xr-x  /usr/share/cacti/include/js/images/ui-icons_ffffff_256x240.png

You missed my comment about systemd requirements. You need to add:
BR: systemd
%{systemd_requires}

... in fact even rpmlint warns about this:
cacti.noarch: W: percent-in-%post
cacti.noarch: W: percent-in-%postun
Comment 7 Zbigniew Jędrzejewski-Szmek 2016-04-14 12:43:59 EDT
(In reply to Morten Stevens from comment #5)
> > Why do you need %patch0 if you have %autosetup? %autosetup should do
> > patching automatically.
> 
> I'm not sure, but the patch was not applied
Looks like a bug in %autosetup.
Comment 8 Zbigniew Jędrzejewski-Szmek 2016-04-14 18:38:07 EDT
Use %license for LICENSE.
Comment 9 Morten Stevens 2016-04-15 09:51:43 EDT
Okay, please check the latest changes:

Updated SRPM: https://mstevens.fedorapeople.org/cacti/cacti-0.8.8g-1.el7.src.rpm
Updated SPEC: https://mstevens.fedorapeople.org/cacti/cacti.spec
Comment 10 Zbigniew Jędrzejewski-Szmek 2016-04-15 09:59:18 EDT
OK, looks good. Package is RE-(APPROVED).

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