Bug 225977

Summary: Merge Review: ksh
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Lubomir Rintel <lkundrak>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: karsten, lkundrak, mhlavink, tsmetana
Target Milestone: ---Flags: lkundrak: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-01-20 20:23:21 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Nobody's working on this, feel free to take it 2007-01-31 19:17:08 UTC
Fedora Merge Review: ksh

http://cvs.fedora.redhat.com/viewcvs/devel/ksh/
Initial Owner: karsten@redhat.com

Comment 1 Lubomir Rintel 2009-01-14 00:21:38 UTC
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 12:07:55 UTC
(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 12:32:20 UTC
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 20:23:21 UTC
(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 08:18:27 UTC
> "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.