Bug 226813

I would be happy to review this package. Look for a full review in a bit. 

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 (BSD)
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:
2cefebf742c190cbc611baded825db64  zsh-4.2.6.tar.bz2
2cefebf742c190cbc611baded825db64  zsh-4.2.6.tar.bz2.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
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. 
See below - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have dist tag
See below - Should package latest version
4 outstanding bugs - check for outstanding bugs on package.

Issues:

1. Should use the correct default build root.

2. rpmlint says:

rpmlint on ./zsh-4.2.6-3.fc7.x86_64.rpm
E: zsh explicit-lib-dependency libcap

Why isn't that getting picked up by rpm? 

E: zsh standard-dir-owned-by-package /etc/skel

Don't own that dir... filesystem already has it. 

E: zsh wrong-script-interpreter /usr/share/zsh/4.2.6/functions/run-help
"/usr/local/bin/zsh"
E: zsh wrong-script-interpreter /usr/share/zsh/4.2.6/functions/checkmail
"/usr/local/bin/zsh"
E: zsh wrong-script-interpreter /usr/share/zsh/4.2.6/functions/zcalc
"/usr/local/bin/zsh"

Perhaps use a sed to change that to remove the local?
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/run-help 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/checkmail 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/zkbd 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/zcalc 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/harden 0644

Might need to be 755?

W: zsh hidden-file-or-dir /etc/skel/.zshrc

Ignore.

W: zsh dangerous-command-in-%postun cp
E: zsh use-tmp-in-%postun

Might be worth looking at a better way to do that...but not sure
off hand what it would be.

rpmlint on ./zsh-4.2.6-3.fc7.src.rpm
W: zsh prereq-use fileutils grep /sbin/install-info
W: zsh make-check-outside-check-section   ZTST_verbose=0 make test

You could move that to a '%check' section...

E: zsh use-of-RPM_SOURCE_DIR
W: zsh macro-in-%changelog version
W: zsh macro-in-%changelog version

Should be %% in changelog for macros.

W: zsh mixed-use-of-spaces-and-tabs (spaces: line 74, tab: line 83)

Use spaces or tabs only?

W: zsh patch-not-applied Patch1: zsh-4.0.6-make-test-fail.patch

Perhaps remove the unneeded patches?

3. 4.3.2 is out, might be worth moving to? They have apparently been working
on multibyte support. Might also address this bug:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=183557

Resetting the flags and such per the new offical review guidelines
https://www.redhat.com/archives/fedora-maintainers/2007-February/msg00682.html

zsh-4.2.6-4.fc7 is now building which should solve most of the above. The big
one still there is usage of RPM_SOURCE_DIR.

1. The buildroot you are using is the one that didn't get approved a few weeks
ago. Hopefully however this will all be moot soon, as their is a proposal up to 
make buildroot back to a reccomendation with several acceptable prefered values. 

2. 
rpmlint on this version says: 

E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/checkmail 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/checkmaile 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/harden 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/hardene 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/run-help 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/run-helpe 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/zcalc 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/zcalce 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/zkbd 0644
E: zsh non-executable-script /usr/share/zsh/4.2.6/functions/zkbde 0644

Your 'sed -ie' should be 'sed -i' ? (it's creating the e ending files)
Also, those should be mode 755? Since they look like executable scripts?

E: zsh use-of-RPM_SOURCE_DIR

Would be nice to fix this... 

E: zsh use-tmp-in-%postun

Not sure what this is about... 

E: zsh wrong-script-interpreter /usr/share/zsh/4.2.6/functions/checkmaile
"/usr/local/bin/zsh"
E: zsh wrong-script-interpreter /usr/share/zsh/4.2.6/functions/run-helpe
"/usr/local/bin/zsh"
E: zsh wrong-script-interpreter /usr/share/zsh/4.2.6/functions/zcalce
"/usr/local/bin/zsh"

The sed -ie issue. ;) 

W: zsh dangerous-command-in-%postun chown

Not sure how else to do that... 

W: zsh hidden-file-or-dir /etc/skel/.
W: zsh hidden-file-or-dir /etc/skel/.
W: zsh hidden-file-or-dir /etc/skel/..
W: zsh hidden-file-or-dir /etc/skel/..
W: zsh hidden-file-or-dir /etc/skel/../skel/.zshrc
W: zsh hidden-file-or-dir /etc/skel/.zshrc
W: zsh hidden-file-or-dir /etc/skel/./.zshrc

Your "%config(noreplace) %{_sysconfdir}/skel/.*" doesn't seem right. 
How about "%config(noreplace) %{_sysconfdir}/skel/.zshrc" ?

W: zsh mixed-use-of-spaces-and-tabs (spaces: line 71, tab: line 107)

Still might be nice to fix. 

W: zsh patch-not-applied Patch1: zsh-4.0.6-make-test-fail.patch

Might be nice to remove. 

W: zsh prereq-use fileutils grep /sbin/install-info

The use of PreReq is deprecated. In the majority of cases, a plain Requires
is enough and the right thing to do. Sometimes Requires(pre), Requires(post),
Requires(preun) and/or Requires(postun) can also be used instead of PreReq.

I think in this case you can ignore fileutils and grep, but install-info will
need to have Requires(post) and Requires(preun). See: 
http://www.fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-47896da5fb2662d75deefeb9ba75145a398515db
for more info. 

 Argh. the sed had been sed -i -e ... but then I merged it, for some stupid
reason. I changed the skel listing to skel/.z*, which should fix that problem.
 The functions are supposed to be sourced, so they don't need to be executable.
I added reqs. -5 should be comming out of the build system soon.

ok, buildroot is now fine. 

>  The functions are supposed to be sourced, so they don't need to be executable.

ok, but then do they need the #!/bin/zsh at the top of each file if they aren't
going to be executed? 

rpmlint says now:

(the non executable script errors from before)

1.E: zsh use-of-RPM_SOURCE_DIR

You use that in the following construct: 

for i in zshrc zlogin zlogout zshenv zprofile; do
    install -m 644 $RPM_SOURCE_DIR/${i}.rhs ${RPM_BUILD_ROOT}%{_sysconfdir}/$i
done

Can't RPM_SOURCE_DIR just be removed from that? The install section should 
be run with a current working dir of the top of the source dir I think...

2. W: zsh mixed-use-of-spaces-and-tabs (spaces: line 72, tab: line 108)

Only a nitpick. Remove if you like while making other changes. 

3. W: zsh patch-not-applied Patch1: zsh-4.0.6-make-test-fail.patch

Remove old patch? 

4. W: zsh prereq-use fileutils grep /sbin/install-info

Those Requires you added shouldn't be needed. You should remove the Requires:
line you added in -5, and also remove the Prereq: line. 
Replacing it with: 

Requires(post): /sbin/install-info
Requires(preun): /sbin/install-info

5. One thing I just noticed that I missed. You should not be using 
%makeinstall if you can avoid it. Can you use 'make DESTDIR=$RPM_BUILD_ROOT
install' instead? 

Thanks for the quick fixes!

> ok, but then do they need the #!/bin/zsh at the top of each file if they aren't
going to be executed? 

 They don't need it, I regard that as a comment saying "this is designed to work
with zsh". I could remove the comment, or more likely make the files executable
just to silence the warning ... I guess. It's kind of hacky but maybe the least
resistence solution.

> Can't RPM_SOURCE_DIR just be removed from that? The install section should 
be run with a current working dir of the top of the source dir I think...

 I'm not sure, I can try it to see. I tried to find documentation that told me
of a better way to do that but couldn't find any ... so just left it alone.

> > 2. W: zsh mixed-use-of-spaces-and-tabs (spaces: line 72, tab: line 108)

> Only a nitpick. Remove if you like while making other changes. 

 I looked at this, IMNSHO rpmlint needs to be fixed for this. The tab is in a
string which is output, removing that would be very bad. I thought about
changing it to use \t ... but I'd rather not change it at all, and rpmlint is
certainly the wrong one here.


 I'll probably remove the patch, Colin added it in Jan, 2005 and it hasn't ever
been enabled AFAICS.

 Is there policy text anywhere about why doing the dep. on /sbin/install-info is
the right approach?
 Dito. %makeinstall ... I'd just like to know (and have it documented here) why
that should change.

 -6 is building now. It doesn't fix the TABs in strings, or use of makeinstall
macro. I think everything else is fixed though.

In reply to comment #8: 

> They don't need it, I regard that as a comment saying "this is designed to work
>with zsh". I could remove the comment, or more likely make the files executable
>just to silence the warning ... I guess. It's kind of hacky but maybe the least
>resistence solution.

Well, I can see it causing confusion for the end user... "hey, this is a script,
I should run it." "what do you mean, Permission Denied?"

> I looked at this, IMNSHO rpmlint needs to be fixed for this. The tab is in a
>string which is output, removing that would be very bad. I thought about
>changing it to use \t ... but I'd rather not change it at all, and rpmlint is
>certainly the wrong one here.

Agreed. ;( 

> Is there policy text anywhere about why doing the dep. on /sbin/install-info
>is the right approach?

Well, nothing set in stone, but they are used in: 
 http://www.fedoraproject.org/wiki/Packaging/ScriptletSnippets
and it seems to make sense to me... whats the alternative? texinfo package?

>Dito. %makeinstall ... I'd just like to know (and have it documented here) why
>that should change.

Yeah, totally understandable. 
http://www.fedoraproject.org/wiki/PackagingDrafts/MakeInstall
http://www.fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002

%makeinstall is acceptable only when the 'make DESTDIR=...' syntax doesn't work
with a package. 

Oh, I was just informed that zsh contains a function called "tetris". 
This is a legal no-no. Can you see if upstream will re-name or remove this? 
Alternately, I suppose we could remove it from the source... 

See: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=224627
for related discussion about the same sort of thing shipped with the emacs package. 

I still see the offending "tetris" function with latest zsh available for F9.

FWIW, /usr/share/zsh/4.3.4/functions/tetris is still present in F10Beta.

So it does. ;( 

James: Can this be removed? 
Adding the legal blocker to see if this is really needing to be fixed.

Well, this is right on the line. The function is tetris, but filename doesn't matter. It doesn't actually display that it is "Tetris" when it runs. Let me talk to the lawyers.

So, the lawyers and I are in agreement here. We do not need to remove the "tetris" file, because:

* It does not describe itself as "Tetris" when it is executed.

Functions and filenames are not a concern for trademark infringement. The notable difference between this and the emacs case is that the emacs "tetris" clearly describes itself as "Tetris" when it is executed.

Lifting FE-Legal.

Wow... here we are 3 years later. :) 

I went ahead and re-ran my review checklist against the current version: 

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:
74c5b275544400082a1cde806c98682a  zsh-4.3.10.tar.bz2
74c5b275544400082a1cde806c98682a  zsh-4.3.10.tar.bz2.orig
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Doc subpackage needed/used. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

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. 
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output. 
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should function as described. 
OK - Should have sane scriptlets. 
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin
OK - check for outstanding bugs on package (merge reviews/rename/re-reviews).  

Issues: 

1. Non blocker, but would '--enable-cap' and '--enable-multibyte' be worth enabling?

2. the tests do indeed hang in local mock here. How does this work in koji?

3. Non blocker, I tried to nuke the %makeinstall with no luck. 
Perhaps just add a note that it's required? 

4. rpmlint says: 

Can't this just be removed now: 

zsh.src:26: E: prereq-use fileutils grep /sbin/install-info
zsh.src: E: specfile-error warning: line 26: prereq is deprecated: Prereq: fileutils grep /sbin/install-info

Misc ignorable issues, fix if you like: 

zsh.src:136: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 136)
zsh.x86_64: E: non-executable-script /usr/share/zsh/4.3.10/functions/sticky-note 0644L /bin/zsh
zsh.x86_64: E: script-without-shebang /usr/share/zsh/4.3.10/functions/mere
zsh.x86_64: E: non-executable-script /usr/share/zsh/4.3.10/functions/calendar_add 0644L /bin/zsh
zsh.x86_64: W: file-not-utf8 /usr/share/doc/zsh-4.3.10/LICENCE
zsh.x86_64: W: dangerous-command-in-%postun cp
zsh.x86_64: E: use-tmp-in-%postun

I don't see any blockers here, just suggestions, so this package is APPROVED. 

Please do consider the above suggestions for the package moving forward. ;) 

You can close this at your leasure...