Bug 225655

rpmlint output is below
Package named correctly
Spec file matches package name
GPL license confirmed, however actual text not included in package
Spec is in English
Spec file is readable
md5sums from upstream match source in SRPM
builds on i386
all build dependencies listed
SHOULD: The description and summary sections in the package spec file should
contain translations for supported Non-English languages, if available.  --
since this a key package, this might be good
Spec file does not use macros for bin, sbin and what not throughout.
Use of prereq, which isn't seen much anymore. Usually, the PreReq tag should be
replaced by plain Requires. 

[builder@rawhide i386]$ rpmlint coreutils-6.7-3.i386.rpm
E: coreutils setuid-binary /bin/su root 04755
E: coreutils non-standard-executable-perm /bin/su 04755
E: coreutils executable-marked-as-config-file /etc/profile.d/colorls.sh
E: coreutils executable-sourced-script /etc/profile.d/colorls.sh 0755
E: coreutils executable-marked-as-config-file /etc/profile.d/colorls.csh
E: coreutils executable-sourced-script /etc/profile.d/colorls.csh 0755
W: coreutils dangerous-command-in-%pre rm
W: coreutils dangerous-command-in-%post mv

[builder@rawhide SRPMS]$ rpmlint coreutils-6.7-3.src.rpm
W: coreutils strange-permission colorls.sh 0775
W: coreutils strange-permission colorls.csh 0775
W: coreutils prereq-use /sbin/install-info
W: coreutils prereq-use grep, findutils
W: coreutils unversioned-explicit-provides stat
W: coreutils unversioned-explicit-obsoletes fileutils
W: coreutils unversioned-explicit-obsoletes sh-utils
W: coreutils unversioned-explicit-obsoletes stat
W: coreutils unversioned-explicit-obsoletes textutils
W: coreutils make-check-outside-check-section make check
W: coreutils mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 13)

Thanks.

> SHOULD: The description and summary sections in the package spec file
> should contain translations for supported Non-English languages, if available.

Historically coreutils.spec has kept its translations in the specspo package, as
other Core packages have done.  What needs to happen now?

> Spec file does not use macros for bin, sbin and what not throughout.

I didn't see any missed bin/sbin macros (note that _bindir is /usr/bin, not
/bin, and similarly for _sbindir), but the %pre scriptlet was missing _datadir
and _infodir.

New package tagged and built as 6.7-4.fc7.

>I didn't see any missed bin/sbin macros (note that _bindir is /usr/bin, not
/bin, and similarly for _sbindir), but the %pre scriptlet was missing _datadir
and _infodir.

My fault.  I was thinking that bindir was /bin and not /usr/bin.  Sorry.

Looks good. 
I would mark is as complete, but this is my first review, and I am still
learning.  I will have somebody else confirm everything is correct and mark it
complete. 

Hey Michael. Thanks for helping out reviewing. :)

I typically use a template to check things, just to make sure I don't miss
anything. Feel free to use my template at: 
http://www.fedoraproject.org/wiki/KevinFenzi/ReviewTemplate

You might also want to look at: 
http://www.fedoraproject.org/wiki/WarrenTogami/ReviewWithFlags
Which is the current review method we are using for these reviews. 

A few things that jump out at me looking at this: 

1. %makeinstall is used. Can 'make DESTDIR=$RPM_BUILD_ROOT install' be used
instead? See the wiki for reasons why %makeinstall is bad. 

2. Does the: 
# Remove these old glibc files on upgrade (bug #84090).
still apply? I can't read that bug, so its hard to tell. 

3. Minor: The Source files should really have 'coreutils-' in front of them. If
you install this src.rpm and want to gather the files from SOURCES it's anoying
to do so. 

4. Is it worth trying to fix bug
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=211615
To fix the loop between pam and coreutils as part of this cleanup/review?

(In reply to comment #2)
> Historically coreutils.spec has kept its translations in the specspo package, 
> as other Core packages have done.  What needs to happen now?

Tim, please DON'T put any translation into coreutils.spec, there's specspo as 
you mentioned and I only know a few packages in Extras which break the typical 
defacto-standard of specspo and include some translations into the spec. Maybe 
these abusing lines within that specs should be killed soon, too...

In reply to comment #1:
1. Fixed, thanks.
2. Applies for upgrades from 6.2 without updates, so I suppose it can go now.
3. Fixed, thanks.
4. Needs further testing and I'm low on spare time this time round :-(

Tagged and built as 6.7-5.fc7.

In reply to comment #6: 

Thanks. Those 4 items all look good. 

Michael: Could you go over the latest version from cvs once more 
with the checklist and see if there is anything else that needs attention?

Sure, but again, I still don't have access to FedoraBugs group.  Oh well. 
Everything looks good.  Rpmlint does show some output, but it is understandable
when profile* is in /etc.  

 OK - Package meets naming and packaging guidelines
 OK - Spec file matches base package name.
 OK - Spec has consistant macro usage.
 OK - Meets Packaging Guidelines.
 OK - License
 OK - License field in spec matches
 OK - License file included in package
 OK - Spec in American English
 OK - Spec is legible.
 OK - Sources match upstream md5sum:
 OK - BuildRequires correct
 OK - Spec handles locales/find_lang
 OK - Package has %defattr and permissions on files is good.
 OK - Package has a correct %clean section.
 OK - Package has correct buildroot
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 OK - Package is code or permissible content.
 OK - Doc subpackage needed/used.
 OK - Packages %doc files don't affect runtime.
 OK - Package compiles and builds on at least one arch.
 OK - Package has no duplicate files in %files.
 OK - Package doesn't own any directories other packages own.
 OK - Package owns all the directories it creates.
 XX - No rpmlint output.  (Not a blocker IMO, I understand and agree with output)
 [builder@rawhide i386]$ rpmlint coreutils-6.7-5.i386.rpm
 E: coreutils setuid-binary /bin/su root 04755
 E: coreutils non-standard-executable-perm /bin/su 04755
 E: coreutils executable-marked-as-config-file /etc/profile.d/colorls.sh
 E: coreutils executable-sourced-script /etc/profile.d/colorls.sh 0755
 E: coreutils executable-marked-as-config-file /etc/profile.d/colorls.csh
 E: coreutils executable-sourced-script /etc/profile.d/colorls.csh 0755
 W: coreutils dangerous-command-in-%post mv

SHOULD Items:

 OK - Should build in mock.
 OK- Should function as described.
 OK - Should have sane scriptlets.
 OK - Should have dist tag
 OK - Should package latest version
 

Forgot rpmlint on the SRPM.  The explicit provide of stat could have version.


[builder@rawhide SRPMS]$ rpmlint coreutils-6.7-5.src.rpm
W: coreutils strange-permission coreutils-colorls.sh 0775
W: coreutils strange-permission coreutils-colorls.csh 0775
W: coreutils unversioned-explicit-provides stat
W: coreutils unversioned-explicit-obsoletes stat

(In reply to comment #8)
>  E: coreutils executable-marked-as-config-file /etc/profile.d/colorls.sh
>  E: coreutils executable-sourced-script /etc/profile.d/colorls.sh 0755
>  E: coreutils executable-marked-as-config-file /etc/profile.d/colorls.csh
>  E: coreutils executable-sourced-script /etc/profile.d/colorls.csh 0755

(In reply to comment #9)
> W: coreutils strange-permission coreutils-colorls.sh 0775
> W: coreutils strange-permission coreutils-colorls.csh 0775

The perms should be fixed. There is no need for sourced files to
be executable.

Tagged and built as 6.7-6.fc7.

Missing
Requires(pre): /sbin/install-info
Requires(preun): /sbin/install-info
Requires(post): grep, /sbin/install-info, coreutils
I am not sure that coreutils in Requires(post) makes sense...

Is 
Requires: grep, findutils
really right?

The versions are certainly wrong in Obsoletes. The latest packaged
version (and versions below) should be obsoleted. The Provides also seems
wrong to me. First of all I guess it should be %{version}-%{release}.
But I am not sure that the current package version should be used 
for the Provides since it is a merge of packages that certainly had 
their own version.

I think it is a bit strange to depend on itself and other packages that
also depends on coreutils and are not really required, that is, anything
else than the shell, gcc and make since there is a bootstrapping issue
otherwise. But I guess that it is impossible to avoid that bootstraping
issue (except by doing complicated things like using busybox...).
I also guess that this issue is also there for gcc, make and bash, so...

There is a huge amount of patches. Many could be submitted (even the pam
one), or have they been submitted and rejected?

/etc, /var and /usr/bin are hardcoded in the spec file, maybe they could
be changed to macros.

It may be relevant to to use -p for files installed, like DIR_COLORS, 
colorls.* ... pam files, since the files are certainly not changed often 
and the timestamp carry some information.

sed could be used instead of
perl -pi -e 's/basic-1//g' tests/stty/Makefile*
and
perl -pi -e 's,/etc/utmp,/var/run/utmp,g;s,/etc/wtmp,/var/run/wtmp,g'
doc/coreutils.texi


Suggestions:

Add a comment above
bzip2 -f9 old/*/C* || :
to explain what it does

Don't use -f for some rm invocations (like rm -f
$RPM_BUILD_ROOT{%_bindir/$i,%_mandir/man1/$i.1}), don't add || : after
bzip2 -f9 old/*/C* || :, and also do bzip2 -9f ChangeLog unconditionally
such that they fail if something changes upstream. At the same time
short-circuit should still work, so care should be taken.

Maybe
[[ -f ChangeLog && -f ChangeLog.bz2  ]] || bzip2 -9f ChangeLog
should be in %install

In the info scriptlets, I suggest removing the .gz from preun and
post, and replace .bz2 by something appropriate. Maybe *, but care
should be taken that it doesn't expands to more than one file.

replace %defattr(-,root,root) by %defattr(-,root,root,-)

Requires(...): fixed
No, doesn't make sense to require coreutils so I haven't added that.

Versioning: fixed

Depending on itself: Er.. can you be more specific?  Which line are you looking at?

Huge amount of patches: there are just 13 patches, and work is being done
upstream to integrate several of them.  Others will never get upstream.

FHS macros: this was addressed in comment #2.

install -p: fixed

sed instead of perl: no need to change this

bzip2 comment: added

rm -f: fixed

||: : fixed

conditional bzip2: fixed

bzip2 moved to %install: fixed

The .bz2 in %pre: this is for legacy upgrades.
The .gz in %preun/%post: fixed

%defattr: fixed

(In reply to comment #13)

> Depending on itself: Er.. can you be more specific?  Which line are you
looking at?

The build is depending on coreutils commands, like mkdir, install, cp,
mv. It also depends on find (in find_lang), bzip2, m4 (through autoconf),
perl. Build dependency on bzip2 and perl could be avoided, but in any
case I can't see how depending on coreutils could avoioded completely. 
It would depend through make, gcc and certainly some libs anyway.

> Huge amount of patches: there are just 13 patches, and work is being done
> upstream to integrate several of them.  Others will never get upstream.

13 patches is a lot, for a package which has an active upstream, 
and patches are not related with integration to fedora. Anyway I 
guess you know much better what to do than me, still it may be worth 
retrying patch submission after time has gone by and things evolved.

> FHS macros: this was addressed in comment #2.

No, it wasn't. I said
  /etc, /var and /usr/bin are hardcoded in the spec file, maybe they could
  be changed to macros.
in comment #2, there is mention of /sbin and /bin. In fact for /etc and
/var it is in fact a bad idea since it is a substitution in a doc file, 
it is better to leave them as is, so you can forget about /etc and
/var, sorry for the noise.

There is still one /usr/bin:
for i in env cut; do ln -sf ../../bin/$i $RPM_BUILD_ROOT/usr/bin; done


Suggestion:
remove / in RPM_BUILD_ROOT/%{_datadir}

> sed instead of perl: no need to change this

This is more a suggestion, but it may be worth trying to avoid unnedeed
build dependency in the hope to reduce further the build base; in my
opinion it would be nice not to have perl in every build.

Also I don't think that /etc/profile.d/ content should be 
(noreplace) and I even think that making it %(config) is dubious.

> > > Requires(post): grep, /sbin/install-info, coreutils
> > > I am not sure that coreutils in Requires(post) makes sense...
> > Depending on itself: Er.. can you be more specific?
> The build is depending on coreutils commands, like mkdir

Are we talking about BuildRequires tags or Requires tags?  Please be specific
with the changes you'd like me to make -- also it would help if you number them. :-)

> There is still one /usr/bin:
> for i in env cut; do ln -sf ../../bin/$i $RPM_BUILD_ROOT/usr/bin; done

Take a look at the wider picture there: we are making compatibility symlinks for
binaries that used to be in /usr/bin but are now in /bin.  We explicitly know
from the history of the packages where these binaries used to be.  Changing a
variable is not going to change that.  Changing /usr/bin here to be %{_bindir}
would be incorrect.

> remove / in RPM_BUILD_ROOT/%{_datadir}

Fixed.

> Also I don't think that /etc/profile.d/ content should be
> (noreplace) and I even think that making it %(config) is dubious.

Fixed.

Tagged and built as 6.7-8.fc7.

(In reply to comment #16)

> > > Depending on itself: Er.. can you be more specific?
> > The build is depending on coreutils commands, like mkdir
> 
> Are we talking about BuildRequires tags or Requires tags?  Please be specific
> with the changes you'd like me to make -- also it would help if you number
them. :-)

I am talking about BuildRequires, and I don't really want that you
do changes, since it seems impossible to me to remove those bootstrapping
issues. But I'd like something like your opinion on that subject.

> > There is still one /usr/bin:
> > for i in env cut; do ln -sf ../../bin/$i $RPM_BUILD_ROOT/usr/bin; done
> 
> Take a look at the wider picture there: we are making compatibility symlinks for
> binaries that used to be in /usr/bin but are now in /bin.  We explicitly know
> from the history of the packages where these binaries used to be.  Changing a
> variable is not going to change that.  Changing /usr/bin here to be %{_bindir}
> would be incorrect.

I wouldn't say incorrect, but not using a macro seems correct.


A suggestions: use the install-info scriptlets from the guidelines, they
are simpler. So replace, in %preun
    [ -f %{_infodir}/%{name}.info.gz ] && \
      /sbin/install-info --delete %{_infodir}/%{name}.info* \
                         %{_infodir}/dir || :
with
    /sbin/install-info --delete %{_infodir}/%{name}.info %{_infodir}/dir || :

and, in %post
[ -f %{_infodir}/%{name}.info* ] && \
  /sbin/install-info %{_infodir}/%{name}.info* %{_infodir}/dir || :
with
/sbin/install-info %{_infodir}/%{name}.info %{_infodir}/dir || :

I find it very strange to use perl to do simple substitutions when a 
sed call does exactly the same.



I don't have any other remark.

BuildRequires: yes, coreutils necessarily poses a bootstrapping problem.  There
isn't really anything that can be done about that.

install-info: okay, done

sed: alright, I've done this too.

Tagged and built as 6.7-9.fc7.

A last comment on spec file, linked with
W: coreutils strange-permission coreutils-colorls.sh 0775
W: coreutils strange-permission coreutils-colorls.csh 0775

you should consider removing the exec perms from those files
in the srpm.

The other rpmlint comments seem good to me.



I have noticed that a test fails, on a fedora devel with a lot of things
installed:

make[4]: Entering directory
`/home/dumas/src/fc-cvs/coreutils/devel/coreutils-6.7/tests/mv'
....
XPASS: acl
....
==============================================================
1 of 36 tests did not behave as expected (1 unexpected passes)
(1 tests were not run)
Please report to bug-coreutils
==============================================================

the source match upstream, but the timestamp is not the same.

> you should consider removing the exec perms from those files
> in the srpm.

I don't seem to have any control over this.  I tried removing those files from
CVS and then adding them back in with fixed permissions, but it didn't work.

> XPASS: acl

This is fine; we patch in ACL support.  When ACL support isn't present the test
is skipped anyway so I think this is something that should be fixed upstream.

> the source match upstream, but the timestamp is not the same.

Next time I update I'll try to remember to use curl -OR.

(In reply to comment #21)
> > you should consider removing the exec perms from those files
> > in the srpm.
> 
> I don't seem to have any control over this.  I tried removing those files from
> CVS and then adding them back in with fixed permissions, but it didn't work.

Ok. Maybe you'll be able to do that after the merge? In any case it
is not a big deal.

> > XPASS: acl
> 
> This is fine; we patch in ACL support.  When ACL support isn't present the test
> is skipped anyway so I think this is something that should be fixed upstream.

Maybe, but this stops the build... Maybe I am wrong and this is not this
test that stops the build?

> Next time I update I'll try to remember to use curl -OR.

Right.

Oh, didn't realise it was stopping the build.  I'll investigate further.

Okay, this has already been fixed in 6.8 which I hope to include in Fedora 7. 
Is that sufficient, or do you need to see the backport (or a coreutils-6.8
package..)?

(In reply to comment #24)
> Okay, this has already been fixed in 6.8 which I hope to include in Fedora 7. 
> Is that sufficient, or do you need to see the backport (or a coreutils-6.8
> package..)?

Do as you like. In case you keep things as is, please comment out
the %check with a comment stating that it should be fixed in 6.8.

coreutils-6.7.tar.bz2.sig should certainly be changed.

It seems to me that the install-info call with * is unsafe
because (from the info page) install-info only takes one file.
So if a file is split it will fail. For the delete of legacy 
sh-utils textutils fileutils it is fine since I guess we can
assume that there is only one file. For the other scriptlets
I think it would be safer without * (install-info should be
able to figure out if the files are compressed).

new rpmlint warning (ignorable in my opinion):
W: coreutils no-version-in-last-changelog

Fixed in CVS.  Will update signature and build when 6.9 is available, and will
also add a comment here when that is done.

Seems to me that everything is right (except the license tag
which is still the old GPL one), this is not a blocker for 
me.

I reassign to me and set feodra review to +, Michael or Kevin
you can reassign to you if you want to, I didn't do most of the 
review.

Shouldn't this bug be closed?