Bug 242310 - Review Request: moreutils - Additional unix utilities
Review Request: moreutils - Additional unix utilities
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Package Reviews List
:
Depends On: 242311
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-03 03:43 EDT by Marc Bradshaw
Modified: 2011-09-19 06:16 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-24 19:24:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Marc Bradshaw 2007-06-03 03:43:01 EDT
First package, sponsor required.

Spec URL: http://marcbradshaw.co.uk/packages/SPECS/moreutils.spec

SRPM URL: http://marcbradshaw.co.uk/packages/SRPMS/moreutils-0.20-1.fc6.src.rpm

Description:  This is a growing collection of the unix tools that nobody thought
 to write thirty years ago.

 So far, it includes the following utilities:
  - isutf8: check if a file or standard input is utf-8
  - sponge: soak up standard input and write to a file
  - ts: timestamp standard input
  - vidir: edit a directory in your text editor
  - vipe: insert a text editor into a pipe
  - combine: combine the lines in two files using boolean operations
  - ifdata: get network interface info without parsing ifconfig output
  - pee: tee standard input to pipes
  - zrun: automatically uncompress arguments to command
  - mispipe: pipe two commands, returning the exit status of the first
Comment 1 Ruben Kerkhof 2007-06-17 04:30:45 EDT
Hi Marc,

It looks like $RPM_OPT_FLAGS are ignored during building:
+ make -j2 'OPTIMIZE=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector 
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -
DSUPPORT_LH7 -DMKSTEMP'
cc -O2 -g -Wall    isutf8.c   -o isutf8
cc -O2 -g -Wall    ifdata.c   -o ifdata
cc -O2 -g -Wall    pee.c   -o pee
cc -O2 -g -Wall    sponge.c   -o sponge
cc -O2 -g -Wall    mispipe.c   -o mispipe

make %{?_smp_mflags} CFLAGS="%{optflags} -DSUPPORT_LH7 -DMKSTEMP"
works better.

furthermore, can you include COPYING and README in %doc?
Comment 2 Marc Bradshaw 2007-06-17 19:15:42 EDT
Hi Ruben,

Thanks for the comments, I have made the modifications as suggested.

New SRPM: http://marcbradshaw.co.uk/packages/SRPMS/moreutils-0.20-2.f7.src.rpm
Comment 3 Marc Bradshaw 2007-06-25 23:41:02 EDT
New SRPM:
http://marcbradshaw.co.uk/packages/SRPMS/moreutils-0.20-2.src.rpm
Comment 4 Ralf Corsepius 2007-06-26 00:33:16 EDT
IMO, /usr/bin/ts and /usr/bin/combine are names likely to clash with other packages.
Comment 5 Marc Bradshaw 2007-06-26 01:16:56 EDT
(In reply to comment #4)
> IMO, /usr/bin/ts and /usr/bin/combine are names likely to clash with other
packages.

On checking those names do not appear in any current fedora packages.  Do you
know of any clashes?
Comment 6 Ralf Corsepius 2007-06-26 01:28:43 EDT
No, I don't know of such clash in Fedora, but a google search indicates
* ts to be used by some tcl packages
* combine to be used by some ImageMagick packages

Apart of this, generally speaking, choosing such "likely to clash names" for a
newly developed package isn't necessarily a clever design.

Comment 7 Marc Bradshaw 2007-06-26 03:24:42 EDT
I appreciate your comments.
moreutils has been in debian for some time and is now also in the ubuntu repo 
and as such the names already have some "history" behind them.
On the one hand there is the issue of the "likely to clash" names but IMO it 
would be more important in this case to keep the names consistant with both 
upstream and with the equivalent package in the other distros.

ImageMagick has not included the combine binary for a significant number of 
versions.
There is a tcl package with a LaTeX component which uses /usr/bin/ts for SUSE 
(ref: http://www.novell.com/products/linuxpackages/suselinux/ts.html).

I would appreciate some further opinions on this issue.
Comment 8 Ralf Corsepius 2007-06-26 04:16:23 EDT
(In reply to comment #7)
> I appreciate your comments.
> moreutils has been in debian for some time and is now also in the ubuntu repo 
> and as such the names already have some "history" behind them.
This only means their QA has failed to recognize the potential dangers and has
slipped through a package they better should not ship.

> On the one hand there is the issue of the "likely to clash" names but IMO it 
> would be more important in this case to keep the names consistant with both 
> upstream and with the equivalent package in the other distros.
I think it would be appropriate to notify upstream to have their choices
revisited and not to include this package until upstream has fixed them.
Comment 10 Hans de Goede 2007-09-17 16:54:59 EDT
Hi Marc,

As discussed by private mail, I'll review your 3 submissions and when they are
all approved I'll sponsor you.

I've done a full review of the latest version and I've found a few issues
besides the potential name clash for ts:

Must FIX:
---------
* change license from "GPL" to "GPLv2" (sponge is GPL version 2 only)
* upstream has 0.24 out, update please
* The weird CPAN comment doesn't make any sense, either remove it or make it 
  make sense
* Please use the BuildRoot from the Packaging Guidelines
* Don't install README and COPYING under %{_datadir}/%{name} instead add them to
  %files like this: "%doc README COPYING" rpm wil then automatically create a 
  dir under /usr/share/doc for them and put them there.
* Don't use %doc for man files.


As for the ts namespace clash, I see 2 options:
1) Ship with upstreams ts name, so that we are consistent with upstream, and 
   rename to ts-stdin if an actual file conflict arises
2) Rename to ts-stdin now

I tend to prefer 1, but if we do 2 now, we avoid pain for end users if we have
to rename later.
Comment 11 Marc Bradshaw 2007-09-17 21:12:31 EDT
Hi Hans,
Thanks again for that.

New SRPM:
http://marcbradshaw.co.uk/packages/moreutils-0.24-1.src.rpm

Regarding the namespace clash, I have gone with option 1 at present as this will
not require existing scripts to be re-written.  If it does need to be renamed
later so be it.  Unless you have any strong feelings to the contrary I suggest
we go with option 1.
Comment 12 Hans de Goede 2007-09-18 15:26:13 EDT
(In reply to comment #11)
> New SRPM:
> http://marcbradshaw.co.uk/packages/moreutils-0.24-1.src.rpm
> 

Almost perfect, please put the %doc in %files under %defattr.

> Regarding the namespace clash, I have gone with option 1 at present as this will
> not require existing scripts to be re-written.  If it does need to be renamed
> later so be it.  Unless you have any strong feelings to the contrary I suggest
> we go with option 1.
> 

I'm fine with going for option 1.

p.s.

Why does this bug depend on 242311? I don't see anything provided by
perl-Time-Duration in the Requires list (including automatic requires) of moreutils?
Comment 13 Marc Bradshaw 2007-09-18 18:59:03 EDT
whoops, moved to the right place now.

the ts command requires them for added functionality, it seems that as they were
being included via an eval statement rpm was missing them from the automatic
requirements list.  I have added both modules to this version as requirements.

http://marcbradshaw.co.uk/packages/moreutils-0.24-2.src.rpm
Comment 14 Hans de Goede 2007-09-19 08:24:30 EDT
Approved, please wait with posting a CVS admin request until we're done with the
other 2 and I've sponsored you.
Comment 15 Marc Bradshaw 2007-09-23 07:35:11 EDT
New Package CVS Request
=======================
Package Name: moreutils
Short Description: Additional unix utilities
Owners: deebs
Branches: F-7
InitialCC: 
Cvsextras Commits: yes
Comment 16 Kevin Fenzi 2007-09-24 12:10:16 EDT
cvs done.
Comment 17 Marc Bradshaw 2008-08-18 20:51:14 EDT
Package Change Request
======================
Package Name: moreutils
New Branches: EL-4 EL-5
Comment 18 Toshio Kuratomi 2008-08-23 15:10:08 EDT
cvs done.
Comment 19 Marc Bradshaw 2011-09-19 06:09:56 EDT
Package Change Request
======================
Package Name: moreutils
Owners: deebs golfu
Comment 20 Jon Ciesla 2011-09-19 06:16:27 EDT
No new branches requested.  If an ownership change is needed, that is done
via pkgdb at https://admin.fedoraproject.org/pkgdb.  Thanks!

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