Bug 225288

Template I am using for review -- thanks KevinFenzi

 - Package meets naming and packaging guidelines
 - Spec file matches base package name.
 - Spec has consistant macro usage.
 - Meets Packaging Guidelines.
 - License
 - License field in spec matches
 - License file included in package
 - Spec in American English
 - Spec is legible.
 - Sources match upstream md5sum:

 - Package needs ExcludeArch
 - BuildRequires correct
 - Spec handles locales/find_lang
 - Package is relocatable and has a reason to be.
 - Package has %defattr and permissions on files is good.
 - Package has a correct %clean section.
 - Package has correct buildroot
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 - Package is code or permissible content.
 - Doc subpackage needed/used.
 - Packages %doc files don't affect runtime.

 - Headers/static libs in -devel subpackage.
 - Spec has needed ldconfig in post and postun
 - .pc files in -devel subpackage/requires pkgconfig
 - .so files in -devel subpackage.
 - -devel package Requires: %{name} = %{version}-%{release}
 - .la files are removed.

 - Package is a GUI app and has a .desktop file

 - Package compiles and builds on at least one arch.
 - Package has no duplicate files in %files.
 - Package doesn't own any directories other packages own.
 - Package owns all the directories it creates.
 - No rpmlint output.
 - final provides and requires are sane:
     (include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo
=; rpm -qp --requires $i; echo; done
      manually indented after checking each line.  I also remove the rpmlib junk
and anything provided by glibc.)

SHOULD Items:

 - Should build in mock.
 - Should build on all supported archs
 - Should function as described.
 - Should have sane scriptlets.
 - Should have subpackages require base package with fully versioned depend.
 - Should have dist tag
 - Should package latest version
 - check for outstanding bugs on package. (For core merge reviews)

Issues:

  1 License file included in package -- License not included
  2 Upstream source and package source  do NOT match.
[builder@rawhide SPECS]$ wget
http://ftp.debian.org/debian/pool/main/a/at/at_3.1.10.tar.gz
--00:12:28--  http://ftp.debian.org/debian/pool/main/a/at/at_3.1.10.tar.gz
Resolving ftp.debian.org... 128.101.240.212
Connecting to ftp.debian.org|128.101.240.212|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 99179 (97K) [application/x-tar]
Saving to: `at_3.1.10.tar.gz'

100%[=======================================>] 99,179       306K/s   in 0.3s

00:12:28 (306 KB/s) - `at_3.1.10.tar.gz' saved [99179/99179]
[builder@rawhide SOURCES]$ md5sum at-3.1.10.tar.gz
a020a2ec32e1d629c0eef91e5728efad  at-3.1.10.tar.gz
[builder@rawhide SOURCES]$ md5sum ../SPECS/at_3.1.10.tar.gz
6e5857e23b3c32ea6995fb7f8989987e  ../SPECS/at_3.1.10.tar.gz
 3 BuildRequires correct  -- Uses Legacy PreReq and BuildReq should be fixed
accoridng to package guidelines.
 4 Should /etc/at.deny have a noreplace option?
 5 Package has correct buildroot of 
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  BuildRoot is not the normal string:
      current package has: %{_tmppath}/%{name}-root
 6 Current package does not build on rawhide fc7 -- at-3.1.10-7  (looks like a
pam patch error)
 7 Uses %makeinstall macro -- see package guidelines for why this is not recommended
 8 Spec file is readable, but has LOTS of commented out older patches.  Do they
still need to be there?
 9 rpmlint not run yet, as package does not build in rawhide

I use rpmlint and fix what I can.
I can't change it in src.rpm wrong sources permission.
rpmlint at-3.1.10-8.fc7.src.rpm
W: at strange-permission atd.init 0775
W: at strange-permission test.pl 0755

In package are "strange" permission. I think they are ok, because at have
special permission for using lock files, pam etc.
rpmlint i386/at-3.1.10-8.fc7.i386.rpm 
E: at non-readable /etc/pam.d/atd 0640
E: at non-standard-dir-perm /var/spool/at/spool 0700
E: at non-standard-dir-perm /var/spool/at 0700
E: at non-readable /etc/at.deny 0600
W: at hidden-file-or-dir /var/spool/at/.SEQ
E: at non-readable /var/spool/at/.SEQ 0600
E: at setuid-binary /usr/bin/at root 04755
E: at non-standard-executable-perm /usr/bin/at 04755
W: at dangerous-command-in-%post chown
W: at service-default-enabled /etc/rc.d/init.d/atd

Should be fixed anything else?

Let me offer some comments on the rpmlint output.

W: at strange-permission atd.init 0775
W: at strange-permission test.pl 0755
  Generally these aren't worth bothering with, but having a file group writable
in your checkout could be problematic.  I don't see that, but I think my umask
doesn't allow it.  Someone should try to understand where this is coming from.

E: at non-readable /etc/pam.d/atd 0640
   I think this is OK, albeit different from what most other packages do.  (They
use 0644).

E: at non-standard-dir-perm /var/spool/at/spool 0700
E: at non-standard-dir-perm /var/spool/at 0700
E: at non-readable /etc/at.deny 0600
E: at non-readable /var/spool/at/.SEQ 0600
   These are necessitated by security.

W: at hidden-file-or-dir /var/spool/at/.SEQ
   That's just the file that at uses; it's OK for it to be hidden.

E: at setuid-binary /usr/bin/at root 04755
E: at non-standard-executable-perm /usr/bin/at 04755
   These are necessary.

W: at dangerous-command-in-%post chown
   I'm not really sure why these are here as opposed to just being part of
%files.  Perhaps rpm would keep creating .SEQ.rpmnew files endlessly otherwise?
 If so then I think it's OK.

W: at service-default-enabled /etc/rc.d/init.d/atd
   It's allowable for a service to be on by default, especially in the case of a
daemon that everyone expects to be there.

Created attachment 148446 [details]
Let rpmbuild strip binaries

Applying this patch and removing "%define debug_package %{nil}" lets rpmbuild
strip the binaries and produce a useful debuginfo package.

Ad comment #3: I was lazy to write all this things, but I really appreciate your
help. I thought the same things. At has some strange permission, but it's
perfectly ok for his functionality.

Marcela Maslanova:  Do you still need more info?  I had a few people IRC look
over the review to help provide you with the information I thought you were
requesting.  Let me know if you need more from me.  

Hi Michael,
I'm waiting for approved review, I think it's everything allright.

>W: at strange-permission atd.init 0775
>W: at strange-permission test.pl 0755
>  Generally these aren't worth bothering with, but having a file group writable
>in your checkout could be problematic.  I don't see that, but I think my umask
>doesn't allow it.  Someone should try to understand where this is coming from.

I see atd.init and test.pl at 0755 permissions in the source.  It looks like
those two files are not in upstream and added to make this package.  So
basically, the question is can they have better perms?  I think the answer is yes. 

I am also still seeing a difference between debian's upstream sum and the source
 used for this package.  I am not sure why that is.  When I unpack both soruces
and run a diff, I see no difference.  

From the package review guidelines:

- MUST: The sources used to build the package must match the upstream source, as
provided in the spec URL. Reviewers should use md5sum for this task.

It was stupid differnce between at_... and at-... Now are the sums the same.

debian source
6e5857e23b3c32ea6995fb7f8989987e  at_3.1.10.tar.gz
cat sources
6e5857e23b3c32ea6995fb7f8989987e  at_3.1.10.tar.gz

Whats the status of this review? 

Michael: If you are reviewing it, can you assign it to yourself and set the
fedora-review flag to ? 

Will try to get to this on the weekend.  Sorry for the LONG delay. 

Re-reviewed. 

Any reason why '%define major_ver 3.1.10' is needed when %{version} is the same
value?

This is not a blocker, just a quetsion.

Everything else looks good.  

APPROVED. 

Package Name: at
Short Description: At allows you to specify when will be run command.
Owners: mmaslano
Branches:

Oop mistake, closing as next release without cvs branch.