Bug 1069988 (naemon)

Ugly spec.

Please cleanup:

1. %if %{defined suse_version}

No SUSE please.

2. %define --> %global

https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

3. Remove these:

Packager: Naemon Core Development Team <naemon-dev>
Vendor: Naemon Core Development Team

https://fedoraproject.org/wiki/Packaging:Guidelines#Tags

4. Remove this:

BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}

5. %if 0%{?el7}%{?fc20}%{?fc21}%{?fc22}

Oh!

So finally there will have such?

%if 0%{?el7}%{?el8}%{?el9}%{?el10}%{?fc20}%{?fc21}%{?fc22}%{?fc23}%{?fc24}%{?fc25}%{?fc26}%{?fc27}%{?fc28}%{?fc29}%{?fc30}???

6. CFLAGS="%{mycflags}" LDFLAGS="$CFLAGS" %configure \
    --prefix="%{_prefix}" \
    --bindir="%{_bindir}" \
    --datadir="%{_datadir}/%{name}" \
    --libdir="%{_libdir}/%{name}" \
    --localstatedir="%{_localstatedir}/lib/%{name}" \
    --sysconfdir="%{_sysconfdir}/%{name}" \
    --mandir="%{_mandir}" \
    --enable-event-broker \
    --without-tests \
    --with-pluginsdir="%{_libdir}/%{name}/plugins" \
    --with-tempdir="%{_localstatedir}/cache/%{name}" \
    --with-checkresultdir="%{_localstatedir}/cache/%{name}/checkresults" \
    --with-logdir="%{_localstatedir}/log/%{name}" \
    --with-initdir="%{_initrddir}" \
    --with-logrotatedir="%{_sysconfdir}/logrotate.d" \
    --with-naemon-user="%{naemonuser}" \
    --with-naemon-group="%{naemongroup}" \
    --with-lockfile="%{_localstatedir}/run/%{name}/%{name}.pid" \
    --with-thruk-user="%{apacheuser}" \
    --with-thruk-group="%{naemongroup}" \
    --with-thruk-libs="%{_libdir}/%{name}/perl5" \
    --with-thruk-tempdir="%{_localstatedir}/cache/%{name}/thruk" \
    --with-thruk-vardir="%{_localstatedir}/lib/%{name}/thruk" \
    --with-httpd-conf="%{_sysconfdir}/%{apachedir}/conf.d" \
    --with-htmlurl="/%{name}"

Please remove the redundant options passed by macro %configure.

rpm -E %configure will help.

7. make dox can also have %{?_smp_mflags}, but I haven't tested it yet, you can have a try.

8. [ "%{buildroot}" != '/' ] && rm -rf %{buildroot}

RPM is better now :) Drop it.

9. # because we globally disabled binary striping, we have to do this manually for some files


Fedora packages must have valid(useful) debuginfo package, please tell us the reason.

10. No %clean section please.

11. No slash after %buildroot macro:

%{buildroot}/ --> %{buildroot}

12. %pre section not good:

http://fedoraproject.org/wiki/Packaging:UsersAndGroups#Dynamic_allocation

13. Too pedantic right? Wwhy not just conditonal lines?

%post core
case "$*" in
  2)
    # Upgrading so try and restart if already running
    # For systemctl systems we need to reload the configs
    # becaues it'll complain if we just installed a new
    # init script
    %if 0%{?el7}%{?fc20}%{?fc21}%{?fc22}
      %systemd_postun
      systemctl condrestart %{name}.service
    %else
      /etc/init.d/%{name} condrestart &>/dev/null ||
    %endif
  ;;
  1)
    %if 0%{?el7}%{?fc20}%{?fc21}%{?fc22}
      %systemd_post %{name}.service
    %else
      chkconfig --add %{name}
    %endif
  ;;
  *) echo case "$*" not handled in postun
esac

14. /var --> %{_localstatedir}

15. %files section:

%doc %{_mandir}/man3/nagexp.3

No %doc please.

and should be 

%{_mandir}/man3/nagexp.3*

Append * to manpages, RPM will compress them.

16. AutoReqProv: no

No this line please, please use filter if you meet problem with perl deps, or this one:

http://fedoraproject.org/wiki/Packaging:Perl#Versioned_MODULE_COMPAT_Requires

Or this one:

https://fedoraproject.org/wiki/Perl/Tips#Filtering_autogenerated_dependencies_does_not_work

Great feedback, updates have been made and tested:

Spec URL:
http://dwittenberg.fedorapeople.org/naemon.spec

SRPM URL:
http://dwittenberg.fedorapeople.org/naemon-0.8.0-1.src.rpm

Koji Task URL:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6573528

Dan

Great feedback, updates have been made and tested:

Spec URL:
http://dwittenberg.fedorapeople.org/naemon.spec

SRPM URL:
http://dwittenberg.fedorapeople.org/naemon-0.8.0-1.src.rpm

Koji Task URL:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6573528

Dan

Rawhide build successful now:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6574683

Whether successful or not, the spec file is amazingly convoluted due to large scriptlets and overuse of %attr.

The scriptlets apparently try to clean up the filesystem upon removal of the package. This will need a very careful review, because preferably you would use %ghost entries in %files lists to track files that get created at runtime and shall be deleted when removing the package.

Printing stuff in scriptlets is a "don't do that", because it isn't guaranteed that any package installer will display that output.


Also relevant:

* https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

* https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

(In reply to Michael Schwendt from comment #5)
> Whether successful or not, the spec file is amazingly convoluted due to
> large scriptlets and overuse of %attr.
> 
> The scriptlets apparently try to clean up the filesystem upon removal of the
> package. This will need a very careful review, because preferably you would
> use %ghost entries in %files lists to track files that get created at
> runtime and shall be deleted when removing the package.
> 
> Printing stuff in scriptlets is a "don't do that", because it isn't
> guaranteed that any package installer will display that output.
> 
> 
> Also relevant:
> 
> * https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
> 
> *
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Packaging_Static_Libraries

Right.

Daniel, Please also do something:

1. Try this in terminal:

rpm -E %perl_vendorarch
rpm -E %_sharedstatedir

And correct the one you've used. Feel free to ask me about any question related.

2. Systemd is now the primary init system, so unless you make conditional lines,(yes for RHEL6 init is still supported) please remove sysv support.

3. You can put all you want to show to the users in a README.Fedora or README.RHEL as %doc! And notice the file in %description.

4. # Move SystemV init-script
mv -f %{buildroot}%{_initrddir}/%{name} %{buildroot}%{_bindir}/%{name}-ctl
%endif

Why? Can't systemd start naemon well?

(In reply to Daniel Wittenberg from comment #4)
> Rawhide build successful now:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=6574683

Since it looks like you made some changes to the spec-file for this build, please bump it's revision and provide me with the new spec / srpm for review / discussion.

Many thanks in advance.

> 2. Systemd is now the primary init system, so unless you make conditional
> lines,(yes for RHEL6 init is still supported) please remove sysv support.
> 
> 3. You can put all you want to show to the users in a README.Fedora or
> README.RHEL as %doc! And notice the file in %description.
> 
> 4. # Move SystemV init-script
> mv -f %{buildroot}%{_initrddir}/%{name} %{buildroot}%{_bindir}/%{name}-ctl
> %endif
> 
> Why? Can't systemd start naemon well?

Right now there's some functions in the init script, like configtest, that are not supported by systemd, so I've left the init there for another so you can run things like:
naemon-ctl configtest

I've also inherited some parts of this script so I'll have to do some digging on the perl side.

Dan

0. Not all issues solved pointed out by Michael and me.

1. Please cleanup the %attr, too many.

2. Systemd scriptlet is still not correct. Please read carefully.

3. 

### Build our documentaiton
cd %{name}-core
make %{?_smp_mflags} dox

-->

make -C %{name}-core dox %{?_smp_mflags}

4. %post core
if [ "$1" = "2" ]; then
    %if 0%{?rhel} >= 7 || 0%{?fedora} >= 20
      %systemd_postun
      systemctl condrestart %{name}.service
    %else
      %{_initrddir}/%{name} condrestart &>/dev/null ||
    %endif

Why does postun scripet appear in %post section?

5. Do we need this?

else
    %logmsg "User \"%{apacheuser}\" does not exist and is not added to group \"%{naemongroup}\". Sending commands to %{name} from the CGIs is not possible."
fi

6. Conflicts:   thruk

What?

7. Requires(preun): %{name}-thruk-libs = %{version}-%{release}
Requires(post): %{name}-thruk-libs = %{version}-%{release}

??

8. %description thruk
This package contains the thruk gui for %{name}

Append dot at the last line.

9. %files devel
%attr(-,root,root) %{_includedir}/%{name}/
%attr(-,root,root) %{_libdir}/%{name}/libnaemon.a
%attr(-,root,root) %{_libdir}/%{name}/libnaemon.la

Not allowed.

10. %attr(0644,root,root) %{_libdir}/%{name}/livestatus.o

Object file?

11. BuildRequires: gd-devel > 1.8
BuildRequires: gd

Duplicate. Should use -devel only.

12.  configure   --without-tests \

Recently regarding the tests in RPM has been discussed heatedly, please tell us the reason.

13. CFLAGS="%{mycflags} -Wformat" LDFLAGS="$CFLAGS"

No %{optflags} used, no %{__global_ldflags} used.

14. Fedora packages have -doc subpackage for documentation. Your opinion?

15. Koji:

http://koji.fedoraproject.org/koji/taskinfo?taskID=6579341

I just found huge tons of bundled perl modules. And this cause the final RPM provides conflicts with system.

Some updates... As thruk developer and naemon core dev like Dan i hopefully can help to change things to fit the fedora requirements.

1. Dan has to look at this

2. Dan has to look at this

3. Fixed upstream

4. can't find that upstream anymore, so i assume its fixed.

5. Fixed upstream

6. This would be the package name of the standalone gui. Its not a official 
fedora package atm. It can go if it violates any packaging guidelines.

7. this was required to workaround dependency and order problems on package removal where the naemon-thruk-libs packages was removed before naemon-thruk.

8. Fixed upstream

9. What exactly is the problem here? Should we just skip the devel package? Or should we provide full %attr?

10. .o is the file extension for nagios/naemon loadable objects.

11. Fixed upstream

12. This does not skip tests entirely, but does not build extented tests which would require additional dependencies. Since full tests are done on travis-ci, we skiped them during packaging.

13. Dan has to look at this

14. I would go for -doc packages. Since we want to include a copy of the html documentation in the future, this would be a good idea.

15. %docs removed upstream and fixed man page globs.

16. The Perl modules are the reason why we used AutoReqProv:no in naemon-thruk and naemon-thruk-libs. In an ideal world, all required perl modules would exist as packages already, then we could just skip the libs package and use requires entirely. Thruk itself looks like a perl module for cleaner development, but is a catalyst perl application, so it does not provide any perl modules.
I could probably rewrite this to use
%global __provides_exclude_from ...
%global __requires_exclude_from ...

17. %if %{defined suse_version}:
We tried to make a universal spec file which fits most systems. We could of course create a fedora only spec file. But i see little benefit, it just creates maintainance overhead. If this is a strict requirement, we could maybe provide a script to automatically remove these tags on building the fedora source packages.

(In reply to Sven Nierlein from comment #10)
> Some updates... As thruk developer and naemon core dev like Dan i hopefully
> can help to change things to fit the fedora requirements.

Thanks!

> 6. This would be the package name of the standalone gui. Its not a official 
> fedora package atm. It can go if it violates any packaging guidelines.

One package conflicts with a package non-existing forever, uh? ;)

> 9. What exactly is the problem here? Should we just skip the devel package?
> Or should we provide full %attr?

Unclear sorry, see:

https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

"Static libraries should only be included in exceptional circumstances. Applications linking against libraries should as far as possible link against shared libraries not static versions."

Thus you'd better remove static libraries.

> 10. .o is the file extension for nagios/naemon loadable objects.

Got it, thanks!

> 12. This does not skip tests entirely, but does not build extented tests
> which would require additional dependencies. Since full tests are done on
> travis-ci, we skiped them during packaging.

Thanks for the clarification!

> 16. The Perl modules are the reason why we used AutoReqProv:no in
> naemon-thruk and naemon-thruk-libs. In an ideal world, all required perl
> modules would exist as packages already, then we could just skip the libs
> package and use requires entirely. Thruk itself looks like a perl module for
> cleaner development, but is a catalyst perl application, so it does not
> provide any perl modules.
> I could probably rewrite this to use
> %global __provides_exclude_from ...
> %global __requires_exclude_from ...

That's dirty,  AutoReqProv:no is not allowed to be used now.

This case matchs bundled libraries, you must remove them and use the system shipped.

> 17. %if %{defined suse_version}:
> We tried to make a universal spec file which fits most systems. We could of
> course create a fedora only spec file. But i see little benefit, it just
> creates maintainance overhead. If this is a strict requirement, we could
> maybe provide a script to automatically remove these tags on building the
> fedora source packages.

Yes, since providing such is nonsense(trust me). SUSE's way of packaging is not good from the view of Fedora sometimes.

This package seems to bundle tons of perl-modules. You must use the system provided versions.

6. ok, it gone.

9. also gone.

16. i removed the AutoReqProv:no and replaced all shiped modules with requires.
There are a only modules missing:
perl(Catalyst::Plugin::Compress)
perl(Catalyst::Plugin::CustomErrorMessage)
perl(Catalyst::Plugin::Redirect
perl(Catalyst::View::Excel::Template::Plus)
perl(Catalyst::View::GD)
perl(Date::Calc::XS)
perl(Excel::Template::Plus)
perl(LWP::Protocol::connect)

How can we proceed here to get those modules packaged for fedora?

17. well, in this case, suse would get packages with fedora guidelines. The only difference are user names and some packages. But ok, its gone.

Besides the missing modules, its looking quite good.

(In reply to Sven Nierlein from comment #13)
> How can we proceed here to get those modules packaged for fedora?
I'd recommend to start submitting for review, the perl-modules packages you are missing.

> Besides the missing modules, its looking quite good.
Well, AFAIS, there still remain many to be resolved details. However I do not see much use in trying to address them at this point in time, esp. before the perl system-integration issues have been resolved.

review requests for missing perl modules submitted:
#1080204
#1080203
#1080201
#1079753
#1079751
#1079749
#1079745
#1079733
#1079732
#1079718
#1069988

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.