Bug 226529

One IMHO-MUST independent of the review itself is, that /etc/cron.deny is not 
owned by the vixie-cron package, but it should be.

That's not review as in guidelines, I'm waiting with fix for whole review.

(In reply to comment #2)
> That's not review as in guidelines, I'm waiting with fix for whole review.

I don't understand exactly what you are meaning here, but 
comments pointing out issues in packages are perfectly fine in
reviews, and should be acted upon.

From a quick glance at the specfile, I have noticed the following
issues:

* Prereq should be changed to the appropriate Requires(...)

* You should use  $RPM_SOURCE_DIR or %SOURCEN, but not both.
 I suggest using %SOURCEN, since it is most common.

* rpm macros should be used more.

* timestamp of data files should be kept with -p

* /etc/pam.d/crond is certainly %config(noreplace)

Suggestion:

In my opinion, /etc/rc.d/init.d/crond shouldn't be marked as
%config.

Complete output from rpmlint. I commented out uninteresting lines

W: vixie-cron summary-ended-with-dot The Vixie cron daemon for executing
specified programs at set times.
W: vixie-cron invalid-license distributable
W: vixie-cron no-url-tag
W: vixie-cron conffile-without-noreplace-flag /etc/pam.d/crond
#W: vixie-cron conffile-without-noreplace-flag /etc/rc.d/init.d/crond
E: vixie-cron executable-marked-as-config-file /etc/rc.d/init.d/crond
#E: vixie-cron non-readable /etc/pam.d/crond 0600
#E: vixie-cron non-standard-dir-perm /var/spool/cron 0700
#E: vixie-cron non-standard-dir-perm /etc/cron.d 0700
#E: vixie-cron setuid-binary /usr/bin/crontab root 06755
#E: vixie-cron setgid-binary /usr/bin/crontab root 06755
#E: vixie-cron non-standard-executable-perm /usr/bin/crontab 06755
#W: vixie-cron service-default-enabled /etc/rc.d/init.d/crond
#W: vixie-cron incoherent-init-script-name crond

Some issues in specfile
 - standardize buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 - use Requires <packagename> instead of Prereq <file>
 - call make with %{?_smp_mflags} (be sure if package has been built correctly.
If not, remove this flag)

Fix in vixie-cron-4.1-75.fc7.

Errors which stay:
W: vixie-cron no-url-tag
E: vixie-cron non-readable /etc/pam.d/crond 0600
E: vixie-cron non-standard-dir-perm /var/spool/cron 0700
E: vixie-cron non-standard-dir-perm /etc/cron.d 0700
E: vixie-cron setuid-binary /usr/bin/crontab root 06755
E: vixie-cron setgid-binary /usr/bin/crontab root 06755
E: vixie-cron non-standard-executable-perm /usr/bin/crontab 06755
W: vixie-cron service-default-enabled /etc/rc.d/init.d/crond
W: vixie-cron incoherent-init-script-name crond
No upstream -> no url.
Permission are needed for daemon safety. 
service-default-enabled -> ok
incoherent-init-script-name -> ok

* appropriate Requires(...) for scriptlets should be used (instead
  of Requires).

* use rpm macros

*
%attr(755,daemon,daemon) /etc/rc.d/init.d/crond
seems wrong to me, it should be owned by root.

* timestamp of data files should be kept with -p

Do you think that helps? The other requires looks to me fine.

Requires(post): /sbin/chkconfig /etc/init.d /sbin/service
Requires(postun): /sbin/chkconfig /etc/init.d /sbin/service
Requires(preun): /sbin/chkconfig /etc/init.d /sbin/service

> %attr(755,daemon,daemon) /etc/rc.d/init.d/crond
Right, I changed it.
> macro
changed
> timestamps
changed

Instead of checking subsys, you could use the standard scriptlet.
It will be safer, too.

if [ $1 = 0 ]; then
        /sbin/service <script> stop >/dev/null 2>&1 || :
        /sbin/chkconfig --del <script>
fi

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e

Similar for the postun scriptlet.

Regarding the Requires(...) in my opinion it should be along:

Requires(post): /sbin/chkconfig coreutils
Requires(postun): /sbin/chkconfig /sbin/service
Requires(preun): /sbin/chkconfig /sbin/service


There are still places in %files and one in install 
where %{_sysconfdir} should be uses, also %_sbindir in %files.
 
RPM_OPT_FLAGS="$RPM_OPT_FLAGS
seems dubious to me. I guess it comes from a patch. I'll review them
later.


If I'm not wrong this package seems not to have any upstream.
Is there a collaboration between linux distro package maintainers, and 
BSD maintainers to share patches and things like that?


A suggestion (a personal preference):
For install call, I always add -mxxx to show clearly the permission
of installed files.
For that reason I prefer install over cp.

> upstream
I think now are crons separate. This cron has too many improvements related to
selinux and that's the main thing, which is patched.

> permission & files
I played a little with permission and macros.

> rpm_opt
is OK. In many cases are rpm_opt_flags in spec file. 

beware, there is a <script> left in %preun... You should really fix it
rapidly before it hits users...

Also the -p on the make install line has the funny side effect
of printing the whole make database...

* There are still places in %files where %_sysconfdir, %localstatedir
could be used, and also here:
install -pm755 %{SOURCE1} $RPM_BUILD_ROOT/etc/rc.d/init.d/crond

* Suggestion:
replace
cp -p %{SOURCE2} $RPM_BUILD_ROOT%{_sysconfdir}/sysconfig/crond
with
install -p -m644 %{SOURCE2} $RPM_BUILD_ROOT%{_sysconfdir}/sysconfig/crond

(In reply to comment #10)
> > upstream
> I think now are crons separate. This cron has too many improvements related to
> selinux and that's the main thing, which is patched.

Given the number of patches, including the selinux patches it is likely 
that a collaboration between the linux and bsd distros maintainers
could help reducing the double work and improve the testing of new
patches. did you approach other maintainers?

> > rpm_opt
> is OK. In many cases are rpm_opt_flags in spec file. 

I'm sorry but I don't understand that comment... Anyway I'll have a look
at the patches, maybe I can come with something more specific.

I have checked the patch, it isn't right. Some are well
isolated and I'm fine with those, but other are a mess.
Some things are added, the code is changed once more, then
it is removed... Please try to fix it as best as possible
and when you're done I could be more specific.

I'm taking officially the review, assigning to myself and
setting to NEEDINFO.

comment #13 
> install -pm755 %{SOURCE1} $RPM_BUILD_ROOT/etc/rc.d/init.d/crond
How could I replace $RPM_BUILD_ROOT/etc -> with %_sysconfdir = /usr/etc ?

comment #14
> upstream
Could you tell me where find upstream, which you've meant?
> option
You can change makefile or spec file, I decided to change spec.

comment #15
It's possibility to rebuild package and change version. But I want to do it
after some more work on cron.

If you want review patches, it's ok, but I think reviewing selinux patches would
be time consuming.

(In reply to comment #17)
> comment #13 
> > install -pm755 %{SOURCE1} $RPM_BUILD_ROOT/etc/rc.d/init.d/crond
> How could I replace $RPM_BUILD_ROOT/etc -> with %_sysconfdir = /usr/etc ?

If you are meaning that you don't want to use %_sysconfdir because
you find it better to hardcode etc since it is where the init system
will search, then ok for me. May deserve a comment in the spec file.
If you want to say something else I haven't understood.

Anyway there are other places where /etc or /var are hardcoded.

> comment #14
> > upstream
> Could you tell me where find upstream, which you've meant?

I haven't meant that there was an upstream. In fact I told the reverse.
It seems to me that upstream is not active anymore. So it also seems to
me that a collaboration between the linux and bsd distros maintainers
could help reducing the double work and improve the testing of new
patches. did you approach other maintainers?

> > option
> You can change makefile or spec file, I decided to change spec.

I don't really understand your comment but if this is about my suggestion,
since it is only a suggestion, it's ok.

> comment #15
> It's possibility to rebuild package and change version. But I want to do it
> after some more work on cron.

> If you want review patches, it's ok, but I think reviewing selinux patches would
> be time consuming.

I don't want to review the the code in patches, but I want you to 
clean them up such that they are indeed reviewable.

For an example of what is not acceptable, I will take the example of the
vixie-cron-4.1.pam/crond.pam file. It is created in
vixie-cron-4.1-_14_pamd_crond.patch
and then changed in 
vixie-cron-4.1-_50-bz178931.patch
vixie-cron-4.1-_56-pam-session-system-auth.patch
vixie-cron-4.1-loginuid.patch

This isn't acceptable. I can see 2 possibilities
* if vixie-cron-4.1-_14_pamd_crond.patch is available somewhere on the 
  internet the patch from internet should be used, and then there could
  be another patch.
* otherwise the file should be created only at one place


I ask you to change the patches such that each file is changed only in 
one patch, and that patches are grouped by features. If you don't see
which patches are problematic I could tell you more precisely, but maybe 
you could begin some cleanings, and I'll recheck after. 

If you really don't have an idea on what you have to do, I can try to
analyse the patches to give you more specific directions.

The patches aren't made one per function, but one per bug. It's important to see
changes between version so I won't change them.

Do you really mean that being able to associate a patch with a bug
is more important that being able to review the patches? That's
awfully wrong. And how do you coordinate with upstream or other
distros packagers if you have one patch per bug? Maintainability
is the most important consideration when it comes to patches
split management (as long as the final code is the same...).
Imagine one second that you leave this mess and somebody else
take over that package. How long will it take for the new maintainer
to understand the patches?

If you really want to associate a patch with a bug you can do 
some duplication (that is keep patches associated with a bug 
together with patch associated with features), either in the 
specfile, or on internet, but a readable patch must be there.

Given the level of disagreement, I guess that the only solution
is to escalate (first to the devel list and then we'll see) if you
don't reconsider your position.

This seems like thing which shouldn't be discussed as part of review as this
should be really left on the package maintainer preference how he organizes
patches. If vixie-cron package really follows patch-per-bug policy I think it
should be allowed to. I use the same policy in the pam package and I'd be very
upset if I wouldn't be allowed to requiring me to always merge patches, patching
the same code.

And of course the best thing would be to create new upstream with coordination
with other distros and merge the patches there.

Once again reviews are also for things that are not in the
guidelines. In that case there is clearly no consensus. I cannot
review the patches, so I cannot approve the package, I will
reassign to somebody who is willing to review the patches as they 
are. Of course I'll keep on looking at other issues.

Just curious, just noticed on my f7 box, /etc/cron.deny is (still) unowned, an
issue raised in comment #1.  Why is this (still) not addressed?  Scratch that,
doesn't matter... just *do* address it, please.  Probably will want to use
something like:
%ghost %config(missingok,noreplace) /etc/cron.deny
and similarly for /etc/cron.allow

Marcela, another thing showed up to me, which has to be fixed. You shouldn't 
use /usr/share/doc/cron - or even if abuse this directory, please OWN this 
directory in any case. But the correct way would be to use %doc which results 
in /usr/share/doc/vixie-cron at a Fedora system...

The --with-pam at %configure has to be optional related to WITH_PAM macro. And 
please keep the WITH_PAM option, don't rip it out :)

Now when the patches were merged into new upstream would be nice to have + for
review. Is there someone brave?

I fixed almost everything what was mentioned.

I suggest using the %bcond_with/%bcond_without macros.

The autotools calls are unuseful now.

RPM_OPT_FLAGS="$RPM_OPT_FLAGS -DLINT -Dlint" also is unused.

The Url: is missing.

You have forked ISC/BSD vixie cron. Now others, including other distros
should also use the fork such that it becomes upstream vixie-cron. 
Did you contact the distros? Also did you advertise the fork on the 
relevant sites, like freshmeat? We really don't want to have a 
fedora/redhat only package.

This is for upstream cron, but i say it here, since upstream is you:

* You should replace the COPYING file with a file stating the license
  of most of the file, a MIT license. There is also a 2 clause BSD
  for popen.c.

* You should reuse the doc/README file and remove what is said below

To use this:...

  and replace with something relevant. Also change the url. You should
  also state clearly what you took as starting point.

* you should move THANKS from doc/ to top directory.

* the doc/FEATURES files should certainly be in the top directory.

* in ChangeLog, the reference should certainly be to doc/CHANGES and
  not simply doc. Also there is a reference to a spec file, but no
  spec file.

* In Makefile.am automake conditionals should be used to add 
  conditionally the -lpam, -lselinux... Those conditionals should
  be set in configure.ac.

* --with-etcdir seems strange to me. There is already sysconfdir?

* selinux could be set in the default case on linux, but there should
  be a AC_ARG_WITH in linux too. Moreover it would be much better
  to have something like what is done for pam, that is autodetection
  of selinux libs.

* also the audit stuff should be detected, not set unconditionally.

* the unconditional AC_DEFINE are very strange. Either they should be
  in a header file, or a --enable-... should be added?

* Use of AC_PREFIX_DEFAULT(/usr) is not a good idea in my opinion.
  Just leave it to autoconf. Same for CFLAGS and LDFLAGS. At least
  if you want to set LDFLAGS,  verify that it is empty.

%bcond_with wasn't working for me. If you have experiences with this macro, I'd
like to see patch of spec file.

Why are autotools calls unuseful?

URL has been added.

Now are pam, selinux and audit optional.

You've true with license, but I decided to change information files in other way.

I can make an announcement on freshmeat, no problem.

Thank you for your suggestions.

Created attachment 240701 [details]
add without_bcond like macros

(In reply to comment #28)

> Why are autotools calls unuseful?

Because everything is already there in the release... that you did...
If it is not there you should redo a release.

> URL has been added.

There is still a missing URL in spec file.

> You've true with license, but I decided to change information files in other way.

??? The COPYING doesn't match the license of the code.

> I can make an announcement on freshmeat, no problem.

Indeed. And I also think that you should contact the debian, suse
and bsd maintainers.

The following isn't useful:
Buildrequires: audit-libs >= 1.4.1

I fixed it in git repository. Now I built it into devel. 

Created attachment 262981 [details]
fix the with_ conditions

%if %{with_pam} doesn't work with bcond_, must be
%if %{with pam}. Just try a rpmbuild --without something.

Thanks for patch, now it is in upstream.

Release of new clone of cron was two weeks ago on freshmeat.

Any other thoughts?

The other comments still hold. 

About the 'upstream' release, there has been some progress, but 
there still seems to be some problematic issues to me.
The release is broken since the autotools generated files don't 
seem to be there. And some are link while they should be copy in 
the released tarball. Or maybe I am missing something?

Also in the release, a proper changelog for upstream should be there.
The spec file changelog serves a completely different purpose.
If the changelog is in git commit messages, you should use the 
proper tool to generate a changelog file from the VCS.

I also propose, in the README to state that the main new features
are selinux, pam and audit support.

I can contact other distro maintainers, but please, before
that, do a proper release.


After this release, please update the spec file such that I can 
test the build with the --without switches.

Hello Patrice,
about this point:
> After this release, please update the spec file such that I can 
> test the build with the --without switches.
are those three --with options of the spec file in use by a Fedora user?
Or is this spec file really used by some other rpm-based distro?
(I'm afraid both answers are "no", so I'd suggest to remove the --with options
from the spec file.)

(In reply to comment #35)
> Hello Patrice,
> about this point:
> > After this release, please update the spec file such that I can 
> > test the build with the --without switches.
> are those three --with options of the spec file in use by a Fedora user?
> Or is this spec file really used by some other rpm-based distro?
> (I'm afraid both answers are "no", so I'd suggest to remove the --with options
> from the spec file.)

It is useful to test upstream builds with the different options
in rpm. And I guess that it is for different versions of fedora? 
RHEL? I don't care that much if they are not here, though.

I am using the opt-out of PAM. Because it really sucks to have thousands of PAM
messages within the syslog when e.g. vnstat via cron is running. Please keep
this option or fix PAM/cron somehow :)

comment #37
I'd like to review any patches from you ;-)

I'd like to close it, because of new package review with new name of package #428007

*** This bug has been marked as a duplicate of bug 428007 ***