Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 225656 - Merge Review: cpio
Summary: Merge Review: cpio
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ruben Kerkhof
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 17:51 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-06-17 06:54:40 UTC
ruben: fedora-review+


Attachments (Terms of Use)
Fix timestamps (deleted)
2007-02-08 22:33 UTC, Ruben Kerkhof
no flags Details | Diff

Description Nobody's working on this, feel free to take it 2007-01-31 17:51:58 UTC
Fedora Merge Review: cpio

http://cvs.fedora.redhat.com/viewcvs/devel/cpio/
Initial Owner: pvrabec@redhat.com

Comment 1 Ruben Kerkhof 2007-02-03 18:09:05 UTC
* RPM name is OK
* Source cpio-2.6.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock
* File list looks OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* BuildRequires: gettext is missing (required to build the translations)
* The %makeinstall macro should not be used
  (wiki: PackagingGuidelines#MakeInstall)
* The package should contain the text of the license
  (wiki: Packaging/ReviewGuidelines)
* Please preserve timestamps when installing files

rpmlint is not silent:

[ruben@odin cpio]$ rpmlint cpio-2.6-23.fc6.src.rpm 
W: cpio prereq-use /sbin/rmt
W: cpio prereq-use /sbin/install-info

Use Requires(post) and Requires(preun) instead

W: cpio mixed-use-of-spaces-and-tabs (spaces: line 99, tab: line 3)



Comment 2 Peter Vrabec 2007-02-07 09:44:42 UTC
If you want to check upgrade candidate too, you can find it at:
http://people.redhat.com/pvrabec/rpms/tar-1.16.1-1.src.rpm

Comment 3 Peter Vrabec 2007-02-07 12:09:28 UTC
Oops, I'm sorry, ignore comment #2

Comment 4 Peter Vrabec 2007-02-07 17:27:16 UTC
fixed in cpio-2.6-24.fc7.

Comment 5 Ruben Kerkhof 2007-02-07 21:23:35 UTC
Looks good.

One thing though, could you preserve the timestamps when installing files (with install -p or cp -p).

Comment 6 Peter Vrabec 2007-02-08 12:14:50 UTC
What do you mean? These is "install -c -p -m 0644" used for man page in spec 
file. Is there any place I don't preserve timestamps?


Comment 7 Jeremy Katz 2007-02-08 14:32:18 UTC
Removal of the bindir, etc definitions at the top of the spec file move the cpio
binary from /bin to /usr/bin which is a bug.

Comment 8 Peter Vrabec 2007-02-08 15:44:59 UTC
fixed in -25.fc7

Comment 9 Ruben Kerkhof 2007-02-08 22:24:26 UTC
Hi Peter,

About the timestamps, the line
install -c -p -m 0644 %{SOURCE1} ${RPM_BUILD_ROOT}%{_mandir}/man1
is redundant, make install already does this, so it can be removed.

test -z "/bin" || mkdir -p -- "/var/tmp/cpio-2.6-25.fc6-root-mockbuild/bin"
  /usr/bin/install -c 'cpio' '/var/tmp/cpio-2.6-25.fc6-root-mockbuild/bin/cpio'

So cpio is installed without preserving the timestamp.

And a new rpmlint error:

Source RPM:
W: cpio mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 86)

I've attached a patch with the fixes.

Comment 10 Ruben Kerkhof 2007-02-08 22:33:25 UTC
Created attachment 147709 [details]
Fix timestamps

Comment 11 Peter Vrabec 2007-02-09 10:24:37 UTC
thnx. Ruben, patch applied.















Comment 12 Peter Vrabec 2007-02-09 13:52:07 UTC
according to 
"make DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p" install"
should we preserve timestamps on files which were rebuilt?

Comment 13 Ruben Kerkhof 2007-02-10 11:32:16 UTC
Preserving timestamps on generated files is pretty useless indeed. But in this case, setting
INSTALL="install -p" takes care of preserving timestamps on the manpages and config files as well.

The only thing the Guidelines say about this is:
When adding file copying commands in the spec file, consider using a command that preserves the files' 
timestamps, eg. cp -p or install -p.

I made a small typo in the patch, could you change the version in the last %changelog entry
from 2.6.26 to 2.6-26?


Comment 14 Peter Vrabec 2007-02-20 16:51:12 UTC
fixed

Comment 15 Ruben Kerkhof 2007-04-20 22:24:49 UTC
Sorry that it took me so long.
I don't see any further blockers so this package is approved.


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