Bug 225609 - Merge Review: bash
Merge Review: bash
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tim Waugh
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 12:44 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-02-08 07:13:51 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 12:44:29 EST
Fedora Merge Review: bash

http://cvs.fedora.redhat.com/viewcvs/devel/bash/
Initial Owner: twaugh@redhat.com
Comment 1 Kevin Fenzi 2007-02-03 15:19:18 EST
I'd be happy to review this package... look for a full review in a bit here. 
Comment 2 Kevin Fenzi 2007-02-03 17:18:59 EST
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.
Comment 3 Ralf Corsepius 2007-02-04 01:26:38 EST
(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.
 
Comment 4 Kevin Fenzi 2007-02-04 08:20:22 EST
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? 
Comment 5 Ralf Corsepius 2007-02-04 12:30:50 EST
(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.
Comment 6 Mamoru TASAKA 2007-02-04 12:47:22 EST
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.
Comment 7 Mamoru TASAKA 2007-02-04 12:53:35 EST
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.
Comment 8 Ville Skyttä 2007-02-04 12:55:16 EST
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
Comment 9 Mamoru TASAKA 2007-02-04 13:00:23 EST
(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 || :)
Comment 10 Tim Waugh 2007-02-05 13:49:43 EST
Changes applied and tagged as 3.2-5.fc7.
Comment 11 Tim Waugh 2007-02-06 09:40:02 EST
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.
Comment 12 Tim Waugh 2007-02-06 09:45:56 EST
(Reassigning back to Kevin -- is that what we're doing?)
Comment 13 Kevin Fenzi 2007-02-06 22:59:32 EST
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?
Comment 14 Kevin Fenzi 2007-02-06 23:01:52 EST
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.
Comment 15 Tim Waugh 2007-02-07 05:27:22 EST
>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.
Comment 16 Kevin Fenzi 2007-02-07 20:34:47 EST
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. 
Comment 17 Tim Waugh 2007-02-08 07:13:51 EST
Thanks!

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