Fedora Merge Review: zsh http://cvs.fedora.redhat.com/viewcvs/devel/zsh/ Initial Owner: james.antill
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...