Bug 225288 - Merge Review: at
Merge Review: at
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Marcela Mašláňová
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-29 16:06 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

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


Attachments (Terms of Use)
Let rpmbuild strip binaries (1.03 KB, patch)
2007-02-20 16:59 EST, Ville Skyttä
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-29 16:06:31 EST
Fedora Merge Review: at

http://cvs.fedora.redhat.com/viewcvs/devel/at/
Comment 1 Michael Stahnke 2007-02-14 01:37:55 EST
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
Comment 2 Marcela Mašláňová 2007-02-20 09:49:32 EST
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?
Comment 3 Jason Tibbitts 2007-02-20 10:50:28 EST
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.
Comment 4 Ville Skyttä 2007-02-20 16:59:19 EST
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.
Comment 5 Marcela Mašláňová 2007-02-21 13:47:22 EST
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.

Comment 6 Michael Stahnke 2007-02-24 12:55:19 EST
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.  
Comment 7 Marcela Mašláňová 2007-02-26 07:29:48 EST
Hi Michael,
I'm waiting for approved review, I think it's everything allright.
Comment 8 Michael Stahnke 2007-03-03 23:39:31 EST
>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.

Comment 9 Marcela Mašláňová 2007-03-05 03:54:44 EST
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
Comment 10 Kevin Fenzi 2007-06-09 00:32:55 EDT
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 ? 

Comment 11 Michael Stahnke 2007-06-21 22:16:52 EDT
Will try to get to this on the weekend.  Sorry for the LONG delay. 
Comment 12 Michael Stahnke 2007-06-30 21:42:49 EDT
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. 
Comment 13 Marcela Mašláňová 2007-08-23 07:37:39 EDT
Package Name: at
Short Description: At allows you to specify when will be run command.
Owners: mmaslano@redhat.com
Branches:
Comment 14 Marcela Mašláňová 2007-08-23 08:02:37 EDT
Oop mistake, closing as next release without cvs branch.

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