Bug 486119 - Review Request: pdksh - A public domain shell implementing ksh-88
Review Request: pdksh - A public domain shell implementing ksh-88
Status: CLOSED NEXTRELEASE
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: Package Review (Show other bugs)
5.4
All Linux
medium Severity medium
: rc
: ---
Assigned To: Stepan Kasal
:
Depends On:
Blocks: 188273 469297 486124
  Show dependency treegraph
 
Reported: 2009-02-18 10:20 EST by Michal Hlavinka
Modified: 2009-05-07 11:55 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-05 03:46:49 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Michal Hlavinka 2009-02-18 10:20:04 EST
Spec URL: http://mhlavink.fedorapeople.org/pdksh.spec
SRPM URL: http://mhlavink.fedorapeople.org/pdksh-5.2.14-31.src.rpm
Description:
Pdksh package is requested by rhbz#469297 . This is review request for copy of package from RHEL-4 branch.
Comment 1 Bill Nottingham 2009-02-18 11:31:11 EST
Is there a reason this is needed since we already have 'real' ksh?
Comment 2 Michal Hlavinka 2009-02-18 11:44:15 EST
(In reply to comment #1)
> Is there a reason this is needed since we already have 'real' ksh?

see history of this: #469297

in short:
customers have scripts working in pdksh in rhel4 that don't work in ksh in rhel5. (pdksh = ksh88 is not 100 % compatible with ksh = ksh93).

They don't want to rewrite their scripts, but they want to use rhel5 so they want pdksh in rhel5.
Comment 4 Stepan Kasal 2009-02-23 13:12:41 EST
OK source files match upstream:
  871106b3bd937e1afba9f2ef7c43aef3  pdksh-5.2.14.tar.gz
OK package meets naming and versioning guidelines.
FAIL specfile is properly named, is cleanly written and uses macros consistently.
  -- please use %{_bindir} where appropriate
OK dist tag; not present, but I think it's not obligatory
FAIL build root is not correct; please use
  BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
OK license field matches the actual license.
OK license is open source-compatible.
FAIL license text NOT included in package.
  Please add the file "LEGAL" to %doc.
OK latest version is being packaged.
OK BuildRequires are proper.
OK compiler flags are appropriate.
OK %clean is present.
OK package builds in mock.
OK package installs properly.
OK debuginfo package looks complete.
FAIL rpmlint is silent on src.rpm and debuginfo rpms,
     but there is one warning on binary rpms:
rpmlint -i pdksh-5*.i386*:
pdksh.i386: W: spurious-executable-perm /usr/share/doc/pdksh-5.2.14/etc/sys_config.sh
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

-- please remove the x bit from that example file.

OK final provides and requires are sane:
$ rpm -qp --provides pdksh-5.2.14-31.i386.rpm
pdksh = 5.2.14-31
$ rpm -qpR pdksh-5.2.14-31.i386c.rpm
/bin/sh
/bin/sh
/bin/sh
libc.so.6
libc.so.6(GLIBC_2.0)
libc.so.6(GLIBC_2.1)
libc.so.6(GLIBC_2.2)
libc.so.6(GLIBC_2.3)
libc.so.6(GLIBC_2.3.3)
libc.so.6(GLIBC_2.3.4)
libc.so.6(GLIBC_2.4)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)

OK %check is NOT present, because it is not present upstream
OK no shared libraries are added to the regular linker search paths.
OK owns the directories it creates.
OK doesn't own any directories it shouldn't.
OK no duplicates in %files.
OK file permissions are appropriate.  (except for the one found by rpmlint)
OK scriptlets look sane
OK code, not content.
OK documentation is small, so no -docs subpackage is necessary.
OK %docs are not necessary for the proper functioning of the package.
OK no headers, pkgconfig files, libtool .la droppings, nor desktop files.

Please fix the four items marked "FAIL."
Comment 5 Michal Hlavinka 2009-02-24 11:46:56 EST
thanks for review

all FAIL items are fixed now

there is another change:
pdksh now uses 'alternatives' for pdksh/ksh shell selection

Spec URL: http://mhlavink.fedorapeople.org/pdksh.spec
SRPM URL: http://mhlavink.fedorapeople.org/pdksh-5.2.14-31.src.rpm

rpmlint for spec, srpm and binary rpms is silent now
Comment 6 Stepan Kasal 2009-03-05 03:46:49 EST
(In reply to comment #5)
> all FAIL items are fixed now

OK, thanks

> there is another change:
> pdksh now uses 'alternatives' for pdksh/ksh shell selection

I see several problems with that:
- the pdksh rpm contains ksh.1.gz which conflicts with ksh.rpm,
- ksh is not yet adapted to use alternatives.

IMHO, this change is not adequate for RHEL-5, I'd just keep the Conflicts line, not using the alternatives mechanism.

But this goes outside the scope of this review.

APPROVED
(there is no fedora_approved flag for rhel bugs, so I'll indicate this by closing the bug)

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