Fedora Merge Review: bash http://cvs.fedora.redhat.com/viewcvs/devel/bash/ Initial Owner: twaugh
I'd be happy to review this package... look for a full review in a bit here.
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 (GPL) OK - License field in spec matches See below - License file included in package OK - Spec in American English OK - Spec is legible. OK - Sources match upstream md5sum: 00bfa16d58e034e3c2aa27f390390d30 bash-3.2.tar.gz 00bfa16d58e034e3c2aa27f390390d30 bash-3.2.tar.gz.1 0e904cb46ca873fcfa65df19b024bec9 bash-doc-3.2.tar.gz 0e904cb46ca873fcfa65df19b024bec9 bash-doc-3.2.tar.gz.1 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. 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. OK - 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 sane scriptlets. OK - Should have dist tag OK - Should package latest version 3 open bugs - check for outstanding bugs on package. Issues: 1. Could ask upstream to include the GPL COPYING file (minor, non blocker). 2. Why the: Prefix: %{_prefix} That should probibly be removed. 3. Buildroot should be set to the standard buildroot. 4. rpmlint, our little pal says: rpmlint on ./bash-3.2-4.fc7.src.rpm W: bash summary-ended-with-dot The GNU Bourne Again shell (bash) version 3.2. Remove . at end. E: bash tag-not-utf8 %changelog E: bash non-utf8-spec-file bash.spec Perhaps run iconv on the spec file to make it utf8? W: bash redundant-prefix-tag Remove prefix. W: bash unversioned-explicit-obsoletes bash2 W: bash unversioned-explicit-obsoletes etcskel W: bash unversioned-explicit-obsoletes bash2-doc W: bash unversioned-explicit-obsoletes bash-doc Are these still needed? W: bash make-check-outside-check-section make check Move 'make check' to a %check section. E: bash use-of-RPM_SOURCE_DIR Should change install to use '-p' and also refer to %SOURCEN instead of SOURCE_DIR. W: bash macro-in-%changelog pre W: bash macro-in-%changelog clean Should use %% for macros in changelog entries. W: bash mixed-use-of-spaces-and-tabs (spaces: line 154, tab: line 98) Pick one of spaces or tabs. E: bash script-without-shebang /usr/share/doc/bash-3.2/scripts/krand.bash E: bash script-without-shebang /usr/share/doc/bash-3.2/scripts/bcsh.sh E: bash script-without-shebang /usr/share/doc/bash-3.2/scripts/precedence E: bash script-without-shebang /usr/share/doc/bash-3.2/scripts/shprompt Should be mode 644? W: bash hidden-file-or-dir /usr/share/man/man1/..1.gz W: bash hidden-file-or-dir /etc/skel/.bash_logout E: bash postin-without-install-info /usr/share/info/bash.info.gz E: bash postin-without-install-info /usr/share/info/bash.info.gz W: bash hidden-file-or-dir /etc/skel/.bashrc W: bash hidden-file-or-dir /etc/skel/.bash_profile W: bash dangerous-command-in-%postun mv Can all be ignored. 5. Should look at the open bugs for the package. In particular this one: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=224567 Requires(post): ncurses Might be required.
(In reply to comment #2) > E: bash postin-without-install-info /usr/share/info/bash.info.gz > E: bash postin-without-install-info /usr/share/info/bash.info.gz .. > Can all be ignored. Can not be ignored - Is a MUSTFIX.
In reply to comment #3: From the comments in the spec, the reason for those is that install-info, which is in the 'info' package, requires bash. So, how can it install info before it installs bash? You run into a looping dependency and it doesnt work. I'd love to hear a more clever way to get around it, but the scriptlets in the spec seemed the solution to me. Do you see any better way?
(In reply to comment #4) > In reply to comment #3: > > From the comments in the spec, the reason for those is that install-info, which > is in the 'info' package, requires bash. So, how can it install info before it > installs bash? You run into a looping dependency and it doesnt work. This is a problem in bootstrapping bash/info - This only is a problem for bootstrapping (building from scratch), but is not much of importance to the end-users. The lack of install-info inside of the rpm breaks each and every end-user's installation, therefore it is a MUST.
Interesting situation. So: A. Anyway bash must be installed first because info requires bash. At the time bash cannot call install-info (and at the time bash.info is useless because info is not used, so not calling install-info is okay) B. Next info is installed. At the time "info" (not bash) has to check if bash.info exists. If it exists (and must exist for this case), "info" must call install-info for bash.info C. Next bash may be upgraded. At the time "bash" must call install-info for bash.info. And the loop problem can be fixed.
So: * If info is not installed, bash doesn't have to call install-info for bash.info.gz * info has to call install-info for bash.info.gz only at the first time when info is needed. * After info is installed bash has to call install-info for bash.info.gz every time it is upgraded.
Sounds like a potential job for triggers, something like: %triggerin -- info # run install-info for the bash stuff here || : %triggerun -- info if [ $2 -eq 0 ] ; then # run install-info --delete for the bash stuff here || : fi
(In reply to comment #8) > Sounds like a potential job for triggers, something like: > > %triggerin -- info > # run install-info for the bash stuff here || : > > %triggerun -- info > if [ $2 -eq 0 ] ; then > # run install-info --delete for the bash stuff here || : > fi Perhaps should work well. Also bash has to have a script for calling install-info for upgrade time (with || :)
Changes applied and tagged as 3.2-5.fc7.
Okay, the install-info triggers caused all sorts of breakage because of adding it to the %post script (comment #9) and as a result adding a prereq for it. install-info requires bash, so it's a dep loop. But this comment in the spec file convinced me that we don't need the triggers either: # ***** bash doesn't use install-info. It's always listed in %{_infodir}/dir # to prevent prereq loops So 3.2-7.fc7, tagged and built just now, is my re-submitted package.
(Reassigning back to Kevin -- is that what we're doing?)
In reply to comment #11: Was that a comment here? Or did you find some other reference to it? In reply to comment #12: yes, assign back to me to recheck, and change the 'fedora-review' flag back to ?. One item I see remaining: %makeinstall is used. See: http://www.fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002 for why this is bad. Does a 'make DESTDIR=$RPM_BUILD_ROOT install' work instead?
Sorry, forgot to flip the assigned and flags back... Change it back to assigned to me, and fedora-review flag to ? when you reply to comment #13.
>In reply to comment #11: >Was that a comment here? Or did you find some other reference to it? Yes, comment #9, last paragraph ("...for calling install-info for upgrade time..."). DESTDIR appears to work, so I've made that change as well. Tagged and built as 3.2-8.fc7.
Excellent. I see no further blockers, so this package is APPROVED. Thanks for the great responsiveness and fixes... I think the plan is to leave these review bugs around assigned to you and in the fedora-review + state until it's determined how to merge them into CVS.
Thanks!