Bug 428007 - Package review: cronie
Package review: cronie
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Nils Philippsen
Fedora Extras Quality Assurance
:
: 226529 (view as bug list)
Depends On:
Blocks: 427660
  Show dependency treegraph
 
Reported: 2008-01-08 12:13 EST by Marcela Mašláňová
Modified: 2013-01-09 23:32 EST (History)
7 users (show)

See Also:
Fixed In Version: cronie-1.0-1.f9
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-30 02:14:15 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
nphilipp: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Marcela Mašláňová 2008-01-08 12:13:00 EST
Please review this package. Bugzilla for old package review of vixie-cron #226529.
Comment 1 Bill Nottingham 2008-01-08 12:19:48 EST
Just judging by the name - ew. fcron?
Comment 2 Marcela Mašláňová 2008-01-08 12:30:54 EST
I forgot. Package could be found here:
http://mmaslano.fedorapeople.org/vixie-cron-ng/

[1] My fork hasn't anything common with fcron.
Comment 3 Bill Nottingham 2008-01-08 12:37:20 EST
The point is - why bother forking the ancient cruft of vixie-cron when there are
other upstream projects with more features, etc.?
Comment 4 Patrice Dumas 2008-01-08 14:51:18 EST
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.

Also in the release, a proper changelog should be there.
If the changelog is in git commit messages, you should use the 
proper tool to generate a changelog file from the VCS.

The upstream tarball cannot be found.
Comment 5 Marcela Mašláňová 2008-01-09 03:53:32 EST
to [3] because I can't say selinux and pam in fcron is ok. I didn't test many
other features and the main reason is the vixie-cron-ng will support inotify,
which is needed for battery saving. I don't see anything ancient on using
inotify ;-)
Comment 6 Marcela Mašláňová 2008-01-09 05:04:29 EST
to [4] I'm sorry that takes so long. I was thought about auto tools and I've
finally attached ./configure into source code. Also CHANGES were generated from
git-log.
Comment 7 Bill Nottingham 2008-01-09 11:24:25 EST
(In reply to comment #5)
> to [3] because I can't say selinux and pam in fcron is ok. I didn't test many
> other features and the main reason is the vixie-cron-ng will support inotify,
> which is needed for battery saving. I don't see anything ancient on using
> inotify ;-)

Sure, but I think it might be better to work with an existing upstream project
on these features if possible (akin to using/extending rsyslog rather than
forking ourselves.) Do we have other distros interested in picking up our fork?
Comment 8 Marcela Mašláňová 2008-01-09 11:36:31 EST
[7] I hope that they will. I was looking in other distribution, which cron they
are using and they have some fork of vixie-cron. I think on the servers everyone
is using some vixie-cron with patches.
Comment 9 Nils Philippsen 2008-01-10 11:24:19 EST
CHECK:
- pkg name possibly in conflict with 3rd clause of BSD license (no promotion
  or endorsement with contributor names) -- search for alternative name is in
  progress
- rpmlint checks return:

  vixie-cron-ng.src:17: W: unversioned-explicit-provides vixie-cron

  --> Possibly change that to "Provides: vixie-cron = 4:4.3" (or whatever else
  vixie-cron version this package replaces, just be sure to list the epoch as
  well)

  vixie-cron-ng.src: W: mixed-use-of-spaces-and-tabs (spaces: line 80, tab:
  line 92)

  --> use either spaces or tabs consistently for indentation, not both

- license text not in %doc, possibly add AUTHORS, CHANGES, README as well
- source matches upstream, but the URL is incorrect. Because the tarball is
  attached to a Trac wiki page, "?format=raw" is needed to actually download
  the tarball. Perhaps add a comment with points to the real URL (as changing 
  the "Source: ..." line won't work with koji, it can't handle CGI parameters)
- possibly unnecessary "BR: automake", you don't use it anywhere in the spec
  file
- macro use inconsistent: use '%{_initrddir}' instead of
  '%{_sysconfdir}/rc.d/init.d' in the spec file
- what purpose does "make ... RPM_OPT_FLAGS="$RPM_OPT_FLAGS -DLINT -Dlint"
  have?
- %postun mustn't fail (append " || :" like in %preun)
- "make install" does much too few which you workaround in the spec file by
  creating directories, copying/moving stuff to the directories, I can help
  with automake/autofoo if you wish so "make install" just works
(possibly followup issues of the last one:
- no need to use %attr() for all the files if "make install" just does "the
  right thing"
- don't move files out of the source/build directory as it breaks "rpm -bi
  --short-circuit", just copy them, or better let them be installed with "make
  install"
- instead of renaming %{_sysconfdir}/pam.d/crond.pam to .../crond, just let
  "make install" do the right thing (i.e. install the file as "crond", not
  "crond.pam")
)

GOOD:
- package meets Fedora naming guidelines
- license (BSD and MIT) OK, matches source
- spec file legible, in am. english
- package compiles on devel (x86_64)
- no missing BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
Comment 10 Marcela Mašláňová 2008-01-11 07:18:22 EST
I've fixed some of mentioned problems. I'm working on better Makefile. 

The package was renamed on cronie (successor of cron) and I didn't find any
problems with this name. git and page on fedorahosted will be renamed, I've
already create a ticket.
Comment 11 Marcela Mašláňová 2008-01-11 07:30:28 EST
And the new source is here: http://mmaslano.fedorapeople.org/cronie/
Comment 12 Patrice Dumas 2008-01-11 08:03:15 EST
I like the name.

I have some comments on upstream.
* I think it would be better to isolate crond.pam, crond.sysconfig
  vixie-cron.init (renamed cronie.init) in something like a contrib
  directory and not install them, they are platform specific.
* you should not use AC_TRY_RUN, but AC_TRY_LINK (or similar) otherwise
  cross compilation is impossible.
* you should use sysconfdir instead of etcdir
* There are some strange unconditional AC_DEFINE.
* The following shouldn't be useful:
crond_LDFLAGS = \
        $(LDFLAGS)
* cp in man/Makefile.am are wrong. automake should be able to figure
  out what to do with these files, and if it cannot there are better
  way to handle them.
* The fact that vixie-cron.init and crond.sysconfig appear in the 
  distribution is complete magic to me
* You should copy the automake/autoconf files, not link them. Like
automake -c
or simply
autoreconf
* there are many things in src/pathnames.h that should be set from
  configure values.
* there is an url missing in the README

I can make patches for the things for which patches make sense.

Is there a version control somewhere for cronie?
Comment 13 Nils Philippsen 2008-01-11 10:28:27 EST
CHECK:
- Summary and description contain bad English, summary is too long, I'd use:

Summary: Cron daemon for executing programs at set times

%description
Cronie contains the standard UNIX daemon crond that runs specified programs at
scheduled times and related tools. It is a fork of the original vixie-cron and
has security and configuration enhancements like the ability to use pam and
SELinux.

- URL in the spec file doesn't match new name (needs action from Fedora
  infrastructure)
- Add the epoch to "Obsoletes: vixie-cron <= 4.3" otherwise upgrading won't
  work (a missing epoch has the same effect as a zero epoch)
- Unnecessary build requirement on automake (you don't use it in the spec file,
  if the Makefiles try to rebuild themselves, there's something wrong)
- %preun mustn't fail, add ' || :' to the chkconfig command as well (it's
  always the last command that defines the exit code of a %pre/%post/un
  scriptlet)
- Get rid of the %triggerpostun, the vixie-cron version where it would trigger
  is older than RHEL2.1

Likely not harmful, but odd:
- I don't know why you have this in %build:

  CFLAGS="$RPM_OPT_FLAGS"; export CFLAGS
  [...]
  %configure \
  [...]

  If you use %configure, it already evaluates $RPM_OPT_FLAGS.
- what purpose does "make ... RPM_OPT_FLAGS="$RPM_OPT_FLAGS -DLINT -Dlint"
  have? Does the Makefile make use of $RPM_OPT_FLAGS (which wouldn't be good
  IMHO)?

I take it your working on the following issues in the course of you Makefile fixes:
- "make install" does much too few which you workaround in the spec file by
  creating directories, copying/moving stuff to the directories, I can help
  with automake/autofoo if you wish so "make install" just works
(possibly followup issues of the last one:
- no need to use %attr() for all the files if "make install" just does "the
  right thing"
- don't move files out of the source/build directory as it breaks "rpm -bi
  --short-circuit", just copy them, or better let them be installed with "make
  install"
- instead of renaming %{_sysconfdir}/pam.d/crond.pam to .../crond, just let
  "make install" do the right thing (i.e. install the file as "crond", not
  "crond.pam")
)

GOOD:
- pkg name issue fixed
- rpmlint checks return nothing
- license text and other documentation in %doc
- consistent use of macros
Comment 14 Nils Philippsen 2008-01-11 10:34:42 EST
(In reply to comment #12)
> I have some comments on upstream.
> * I think it would be better to isolate crond.pam, crond.sysconfig
>   vixie-cron.init (renamed cronie.init) in something like a contrib
>   directory and not install them, they are platform specific.

IMO contrib/ directories are usually for stuff contributors submit but upstream
doesn't plan to support (which clearly is not the case here). I'd add e.g.
--with-pam and --with-sysvinit to configure.ac and install the files if the user
wants it. Marcela, shout if you want help with that.

> * The fact that vixie-cron.init and crond.sysconfig appear in the 
>   distribution is complete magic to me

What do you mean?
Comment 15 Marcela Mašláňová 2008-01-11 10:41:37 EST
[12] Thanks, the git archive could be found on
https://hosted.fedoraproject.org/vixie-cron

[9] source could be also from my page http://mmaslano.fedorapeople.org/cronie/
Comment 16 Patrice Dumas 2008-01-11 10:54:50 EST
(In reply to comment #14)
> (In reply to comment #12)
> > I have some comments on upstream.
> > * I think it would be better to isolate crond.pam, crond.sysconfig
> >   vixie-cron.init (renamed cronie.init) in something like a contrib
> >   directory and not install them, they are platform specific.
> 
> IMO contrib/ directories are usually for stuff contributors submit but upstream
> doesn't plan to support (which clearly is not the case here). I'd add e.g.
> --with-pam and --with-sysvinit to configure.ac and install the files if the user
> wants it. Marcela, shout if you want help with that.

Yes, that would be another possibility. And indeed contrib is a bad
name for a directory holding those files. Maybe data or utils?
In any case they shouldn't be installed in the default case since
they are too platform dependent.

> > * The fact that vixie-cron.init and crond.sysconfig appear in the 
> >   distribution is complete magic to me
> 
> What do you mean?

I may have missed something but they are not in any variable in Makefile.am
nor in EXTRA_DIST...
Comment 17 Marcela Mašláňová 2008-01-11 11:41:50 EST
[14] with-pam is defined, I can add with-sysvinit. 
[15] I can move these files into some /data. I think you mean installing
crond.pam etc. with Makefile.am. 
Comment 18 Rahul Sundaram 2008-01-13 04:16:22 EST
Is it the plan to replace vixie cron with this by default in Fedora 9? If so,
following the feature process would be appropriate

http://fedoraproject.org/wiki/Features/Policy
Comment 19 Nils Philippsen 2008-01-14 03:42:37 EST
(In reply to comment #18)
> Is it the plan to replace vixie cron with this by default in Fedora 9? If so,
> following the feature process would be appropriate
> 
> http://fedoraproject.org/wiki/Features/Policy

As we already had this forked version (albeit under the old name vixie-cron) in
F-8, this is merely a re-packaged version of that with the most user-visible
change being the change of the package name. I don't think that counts as a new
feature (but I may be wrong).
Comment 20 Marcela Mašláňová 2008-01-14 11:11:23 EST
[18] Fork is already in rawhide, but now is renaming of package needed. I have
to also in this case go through Features/Policy?
Comment 21 Marcela Mašláňová 2008-01-15 09:32:59 EST
I think the upstream notes should move on upstream page
https://fedorahosted.org/cronie/ and here should be discussed only packaging,
rpmlint output and so on.
Comment 22 Marcela Mašláňová 2008-01-17 08:28:17 EST
I try to fix the makefile and configure according to comments #12 an #13, but I
didn't find how to install files like cronie.init into system like
/etc/sysconfig/crond (in Makefile.ac). I'll be really happy for a patch.

The files crond.sysconfig and crond.pam could be in directory f.e. /etc, if it's
really needed.

Did anyone know if the announcement in Features is needed?
Comment 23 Marcela Mašláňová 2008-01-17 08:36:31 EST
Forgot, updated source are here: http://mmaslano.fedorapeople.org/cronie and git
is here: http://git.fedorahosted.org/git/cronie.git
Comment 24 Marcela Mašláňová 2008-01-25 07:58:46 EST
Ping? The autotools are really fixed. Now is nothing wrong with packaging or
spec file. I think there are now other serious problems, which could slower the
review of package.
Comment 25 Marcela Mašláňová 2008-01-25 07:59:22 EST
http://mmaslano.fedorapeople.org/cronie/
Comment 26 Nils Philippsen 2008-01-25 09:35:35 EST
CHECK:
- Add the epoch to "Obsoletes: vixie-cron <= 4.3" otherwise upgrading won't
  work (a missing epoch has the same effect as a zero epoch)
- Unnecessary build requirement on automake (you don't use it in the spec file,
  if the Makefiles try to rebuild themselves, there's something wrong)
- %preun mustn't fail, add ' || :' to the chkconfig command as well (it's
  always the last command that defines the exit code of a %pre/%post/un
  scriptlet). Alternatively, just append "exit 0" to all the scriptlets.
  See the Fedora Wiki for details ("Syntax" section):
  http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

(I've mentioned the issues above in comment #13 already)

- You unconditionally remove $RPM_BUILD_ROOT%{_sysconfdir}/pam.d/crond.pam in
  the %install section (the %if conditional is commented out). You probably
  should remove the comments so that the file only gets removed if %{with_pam} 
  isn't defined, and change the command to:

rm -f $RPM_BUILD_ROOT%{_sysconfdir}/pam.d/crond

  I.e. the file name is "crond", not "crond.pam" and recursive removal isn't
  needed as it's not a directory.

GOOD:

- Summary and description fixed
- URL fixed (will this be the final location?)
- Obsolete %triggerpostun removed
- Unnecessary use of $RPM_OPT_FLAGS removed
- Files aren't moved out of the source/build directory

Have you checked whether you can leave out the individual %attr flags in the
%files section, i.e. if "make install" installs the files with the proper 
%permissions?
Comment 27 Tomas Mraz 2008-01-25 09:53:56 EST
(In reply to comment #26)
> CHECK:
> - Add the epoch to "Obsoletes: vixie-cron <= 4.3" otherwise upgrading won't
>   work (a missing epoch has the same effect as a zero epoch)
Not really, this is true only the other way around - if it is not specified in
comparsions for obsoletes/requires etc. it is not compared. So it doesn't matter
what epoch the vixie-cron package is. So this is OK.
Comment 28 Marcela Mašláňová 2008-01-25 10:17:05 EST
> - Add the epoch to "Obsoletes: vixie-cron <= 4.3" otherwise upgrading won't
>  work (a missing epoch has the same effect as a zero epoch)
It's ok (comment #27)
> Unnecessary build requirement on automake (you don't use it in the spec file,
>  if the Makefiles try to rebuild themselves, there's something wrong)
Removed.
> - %preun mustn't fail, add ' || :' to the chkconfig command as well (it's
>  always the last command that defines the exit code of a %pre/%post/un
>  scriptlet). Alternatively, just append "exit 0" to all the scriptlets.
>  See the Fedora Wiki for details ("Syntax" section):
>  http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
Added || :

> - You unconditionally remove $RPM_BUILD_ROOT%{_sysconfdir}/pam.d/crond.pam in
>  the %install section (the %if conditional is commented out). You probably
>  should remove the comments so that the file only gets removed if %{with_pam} 
The autotools can install crond in the correct place. If someone change it in
spec without pam, then the installed file will be removed. I renamed false
cron.pam on crond.

> Have you checked whether you can leave out the individual %attr flags in the
> %files section, i.e. if "make install" installs the files with the proper 
> %permissions?
I removed as many permissions as I dare.

I uploaded correct spec file and I'd also built the package in mock.
Comment 29 Nils Philippsen 2008-01-25 10:29:48 EST
(In reply to comment #27)
> (In reply to comment #26)
> > CHECK:
> > - Add the epoch to "Obsoletes: vixie-cron <= 4.3" otherwise upgrading won't
> >   work (a missing epoch has the same effect as a zero epoch)
> Not really, this is true only the other way around - if it is not specified in
> comparsions for obsoletes/requires etc. it is not compared. So it doesn't matter
> what epoch the vixie-cron package is. So this is OK.
> 

I just tried it, updating to my "test-new" package wouldn't get the epoched
"test-old" package removed:

nils@gibraltar:~> rpm -qa test\*
test-old-4.2-1
test-new-1-1
nils@gibraltar:~> rpm -q --qf '%{epoch}:%{v}-%{r}\n' test-old
4:4.2-1
nils@gibraltar:~> rpm -q --obsoletes test-new
test-old <= 4.3

Comment 30 Nils Philippsen 2008-01-25 10:35:51 EST
(In reply to comment #28)
> > - Add the epoch to "Obsoletes: vixie-cron <= 4.3" otherwise upgrading won't
> >  work (a missing epoch has the same effect as a zero epoch)
> It's ok (comment #27)

Unforunately not (comment #29 ;-). Otherwise the package is okay now.
Comment 31 Marcela Mašláňová 2008-01-25 10:44:21 EST
Now I'm convinced. I had test it myself. So I updated again.

And the URL, I have to use url from my web page, because the fedorahosted isn't
functional yet.
Comment 32 Nils Philippsen 2008-01-25 11:03:12 EST
(In reply to comment #31)
> And the URL, I have to use url from my web page, because the fedorahosted isn't
> functional yet.

I thought so. APPROVED.

Comment 33 Tomas Mraz 2008-01-25 11:05:50 EST
I'm sorry for the mistake about the epoch. It works the way I wrote for release
but apparently not for epoch.
Comment 34 Patrice Dumas 2008-01-26 03:37:35 EST
I may have missed something but the source url doesn't work for me,
http://mmaslano.fedorapeople.org/cronie/cronie-1.0.tar.gz
cannot be found, maybe because fedorahosted is down, but the source
from the home page doesn't look quite like the source in the 
srpm.

Anyway it looks like the most importants and blocking comments 
I had about upstream are fixed in the source found in the srpm.
Comment 35 Marcela Mašláňová 2008-01-28 02:36:02 EST
Thanks all for comment. Everything fixed.
Comment 36 Marcela Mašláňová 2008-01-28 02:38:42 EST
New Package CVS Request
=======================
Package Name: cronie
Short Description: time scheduler
Owners: mmaslano
Cvsextras Commits: yes
Comment 37 Dennis Gilmore 2008-01-28 11:00:14 EST
Please use the template completely  including braches and InitialCC  I cant 
process this because i dont know what braches you want the package for 
Comment 38 Marcela Mašláňová 2008-01-29 02:26:01 EST
I didn't write branch, because it's only for rawhide, but ok here it is.

New Package CVS Request
=======================
Package Name: cronie
Short Description: task scheduler
Owners: mmaslano@redhat.com
Branches: rawhide
Cvsextras Commits: yes
Comment 39 Kevin Fenzi 2008-01-29 12:42:11 EST
Please use fedora account name in Owners field. 

cvs done. 
Comment 40 Marcela Mašláňová 2008-01-30 02:14:15 EST
Ok, owners should be only: mmaslano

Thank you for quick cvs branching.
Comment 41 Jesse Keating 2008-01-30 09:58:56 EST
The obsoletes is still wrong.  You're providing /and/ obsoleting the same
vixie-cron version:

Provides: vixie-cron = 4:4.3
Obsoletes: vixie-cron <= 4:4.3

You just need to take the '=' out of the Obsoletes and you'll likely be fine.

Although, the latest vixie-cron build is 4:4.3-2.  This means the existing
vixie-cron won't actually be obsoleted:

$ rpmdev-vercmp 4:4.3 4:4.3-2.fc9
4:4.3-2.fc9 is newer

Please fix your obsoletes/provides.
Comment 42 Tomas Mraz 2008-01-30 10:18:55 EST
(In reply to comment #41)
> The obsoletes is still wrong.  You're providing /and/ obsoleting the same
> vixie-cron version:
> 
> Provides: vixie-cron = 4:4.3
> Obsoletes: vixie-cron <= 4:4.3
> 
> You just need to take the '=' out of the Obsoletes and you'll likely be fine.
> 
> Although, the latest vixie-cron build is 4:4.3-2.  This means the existing
> vixie-cron won't actually be obsoleted:
> 
> $ rpmdev-vercmp 4:4.3 4:4.3-2.fc9
> 4:4.3-2.fc9 is newer

This is OK - see http://marc.info/?l=fedora-devel-list&m=119679575625910&w=2

But the provides should provide something newer - 
So use Provides: vixie-cron = 4:4.3.1 or something like that.
Comment 43 Peter Lemenkov 2009-09-26 11:21:31 EDT
*** Bug 226529 has been marked as a duplicate of this bug. ***

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