Bug 225977 - Merge Review: ksh
Merge Review: ksh
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:17 EST by Nobody's working on this, feel free to take it
Modified: 2009-01-21 03:18 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-20 15:23:21 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:17:08 EST
Fedora Merge Review: ksh

http://cvs.fedora.redhat.com/viewcvs/devel/ksh/
Initial Owner: karsten@redhat.com
Comment 1 Lubomir Rintel 2009-01-13 19:21:38 EST
1.) Spec file legibility: No useless comments please.

#ExclusiveArch:  x86_64
#ExcludeArch:  ia64
#Patch0:       ksh-20041225-gcc4.patch
#%patch0 -p1 -b .gcc4
...

2.) If you temporarily disable a patch, please remove it only from %prep, not the tag itself, so that the patch is bundled with srpm:

# for debugging only:
#Patch100:     ksh-20060124-iffedebug.patch

3.) Use macros. Replace /usr/bin with %{_bindir}

mkdir -p $RPM_BUILD_ROOT{/bin,/usr/bin,%{_mandir}/man1}
/usr//bin/ksh
ln -sf /bin/ksh $RPM_BUILD_ROOT/usr/bin/ksh
...

4.) Use %find_lang for locales

%files
%{_datadir}/locale/*/LC_MESSAGES/*

5.) RPMlint:

ksh.src:28: W: unversioned-explicit-provides ksh93
ksh.src:29: W: unversioned-explicit-obsoletes ksh93

Though it is advisable to version I think it's ok if you leave these as they are; this had been unversioned for some time already and ksh93 is probably won't coming back anyways.

ksh.src: W: patch-not-applied Patch4: ksh-20080725-ttou.patch

Either remove the file, or explain why is it disabled.

6.) Upstream the patches

Please submit the patches to usptream & comment on status.
https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

7.) Latest upstream release

Latest release seems to be 2008-11-04. Please update.

8.) Avoid absolute symlinks

ln -sf /bin/ksh $RPM_BUILD_ROOT/usr/bin/ksh
Comment 2 Michal Hlavinka 2009-01-20 07:07:55 EST
(In reply to comment #1)
> 1.) Spec file legibility: No useless comments please.
> 
> #ExclusiveArch:  x86_64
> #ExcludeArch:  ia64
> #Patch0:       ksh-20041225-gcc4.patch
> #%patch0 -p1 -b .gcc4
> ...

fixed: removed

> 2.) If you temporarily disable a patch, please remove it only from %prep, not
> the tag itself, so that the patch is bundled with srpm:
> 
> # for debugging only:
> #Patch100:     ksh-20060124-iffedebug.patch

fixed: removed
 
> 3.) Use macros. Replace /usr/bin with %{_bindir}
> 
> mkdir -p $RPM_BUILD_ROOT{/bin,/usr/bin,%{_mandir}/man1}
> /usr//bin/ksh
> ln -sf /bin/ksh $RPM_BUILD_ROOT/usr/bin/ksh
> ...

fixed: now uses %{_bindir}

> 4.) Use %find_lang for locales
> 
> %files
> %{_datadir}/locale/*/LC_MESSAGES/*

fiexed: locales are no longer supported by upstream - removed
 
> 5.) RPMlint:
> 
> ksh.src:28: W: unversioned-explicit-provides ksh93
> ksh.src:29: W: unversioned-explicit-obsoletes ksh93
> 
> Though it is advisable to version I think it's ok if you leave these as they
> are; this had been unversioned for some time already and ksh93 is probably
> won't coming back anyways.

didn't decided what to do with these two yet

> ksh.src: W: patch-not-applied Patch4: ksh-20080725-ttou.patch
> 
> Either remove the file, or explain why is it disabled.

fixed: removed, no longer needed

> 6.) Upstream the patches
> 
> Please submit the patches to usptream & comment on status.
> https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

fixed: only one (Fedora specific) patch remains

> 7.) Latest upstream release
> 
> Latest release seems to be 2008-11-04. Please update.

fixed: updated
 
> 8.) Avoid absolute symlinks
> 
> ln -sf /bin/ksh $RPM_BUILD_ROOT/usr/bin/ksh

could you please point me to some packaging guidelines or something about why should I avoid them? ln -sf ../../bin/ksh seems to me pretty ugly to do it without knowing the reason. thanks
Comment 3 Michal Hlavinka 2009-01-20 07:32:20 EST
also this is fixed:

# rpmlint x86_64/ksh-20081104-1.fc11.x86_64.rpm 

ksh.x86_64: W: dangerous-command-in-%postun mv
Comment 4 Lubomir Rintel 2009-01-20 15:23:21 EST
(In reply to comment #2)

Thanks for quick reply and addressing the problems!
/me is much impressed

> > 8.) Avoid absolute symlinks
> > 
> > ln -sf /bin/ksh $RPM_BUILD_ROOT/usr/bin/ksh
> 
> could you please point me to some packaging guidelines or something about why
> should I avoid them? ln -sf ../../bin/ksh seems to me pretty ugly to do it
> without knowing the reason. thanks

Apart from that I don't see what's ugly about "ln -sf ../../bin/ksh", you're probably right that the guidelines don't mandate this. At least I could not find anything. Only piece of information I was able to dig was a rpmlint warning, which is pretty non-specific:

"Absolute symlinks are problematic eg. when working with chroot environments."

I can't think of a real world-scenario where this would cause serious trouble. (Not considering "Imagine I have a complete tree somewhere and I do `something' to the linked file, such as write to it (there actually are tools that write to binaries, such as prelink or execstack)"). So, while I strongly urge you to get rid of that absolute symlink, it seems not to be mandatory.

By the way -- how about removing it? What's ksh in /usr/bin good for? We're probably early enough in rawhide to have enough time to test if it breaks something.
Comment 5 Michal Hlavinka 2009-01-21 03:18:27 EST
> "Absolute symlinks are problematic eg. when working with chroot environments."

yes, but because only common directory for /bin/ksh and /usr/bin/ksh is / directory, so chroot will never work here even with relative path

I think there is no package that require 'ksh' at all, so yes... I'll remove this link.

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