Bug 226526 - Merge Review: vim
Merge Review: vim
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ruben Kerkhof
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:15 EST by Nobody's working on this, feel free to take it
Modified: 2008-11-01 19:45 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-01 19:45:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ruben: fedora‑review+


Attachments (Terms of Use)

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

http://cvs.fedora.redhat.com/viewcvs/devel/vim/
Initial Owner: karsten@redhat.com
Comment 1 Karsten Hopp 2007-02-06 07:18:45 EST
vim-7.0.191-1.fc7 has lots of spec file fixes, please don't review older releases
Comment 2 Robert Scheck 2007-02-18 17:42:19 EST
/etc/profile.d/vim.{sh,csh} is 755, but should be 644:

W: vim-enhanced conffile-without-noreplace-flag /etc/profile.d/vim.csh
W: vim-enhanced conffile-without-noreplace-flag /etc/profile.d/vim.sh
E: vim-enhanced executable-marked-as-config-file /etc/profile.d/vim.sh
E: vim-enhanced executable-sourced-script /etc/profile.d/vim.sh 0755
E: vim-enhanced executable-marked-as-config-file /etc/profile.d/vim.csh
E: vim-enhanced executable-sourced-script /etc/profile.d/vim.csh 0755
Comment 3 Karsten Hopp 2007-02-21 10:52:57 EST
fixed in vim-7.0.195-2.fc7
Comment 4 Karsten Hopp 2007-03-13 11:37:21 EDT
What's the status of this review ? Approved ?
Comment 5 Ruben Kerkhof 2007-03-18 08:11:35 EDT
Hi Karsten,

Needs work:
* BuildRequires: perl should not be included
  (wiki: PackagingGuidelines#Exceptions)
* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)
* rpmlint of vim-common: 
* The package should contain the text of the license
  (wiki: Packaging/ReviewGuidelines)
* Each %files section should have a %defattr line
  (wiki: Packaging/ReviewGuidelines)
* Desktop file: vendor should be fedora
  (wiki: PackagingGuidelines#desktop)
* Scriptlets: missing update-desktop-database
  (wiki: ScriptletSnippets)
* The package owns /usr/share/man/fr, which is a standard directory
  (wiki: Packaging/ReviewGuidelines)
* The package owns /usr/share/man/fr/man1, which is a standard directory
  (wiki: Packaging/ReviewGuidelines)
* The package owns /usr/share/man/it, which is a standard directory
  (wiki: Packaging/ReviewGuidelines)
* The package owns /usr/share/man/it/man1, which is a standard directory
  (wiki: Packaging/ReviewGuidelines)
* The package owns /usr/share/man/pl, which is a standard directory
  (wiki: Packaging/ReviewGuidelines)
* The package owns /usr/share/man/pl/man1, which is a standard directory
  (wiki: Packaging/ReviewGuidelines)

Some minor things:
* Please use BuildRequires instead of Buildrequires everywhere
* Preserve timestamps when installing non-generated files
* Please run rpmlint on all rpms and fix the warnings and errors. The list is a bit long to include here.

Comment 6 Karsten Hopp 2007-04-16 11:30:28 EDT
ok, I'm now down to just a few warnings.
Most of them are like this one:
vim-common file-not-utf8 /usr/share/man/ru.KOI8-R/man1/vimdiff.1.gz

This file isn't intented to be UTF8, it's KOI8-R encoded.

vim-enhanced and vim-X11 report dangling symlinks, but this becomes a normal 
symlink when the required vim-common is installed.

vim -minimal has no documentation, I'd like to keep it this way. Maybe I'll add
the license, but nothing more.
Comment 7 Ruben Kerkhof 2007-04-17 17:11:11 EDT
Hi Karsten,

> ok, I'm now down to just a few warnings.
> Most of them are like this one:
> vim-common file-not-utf8 /usr/share/man/ru.KOI8-R/man1/vimdiff.1.gz

> This file isn't intented to be UTF8, it's KOI8-R encoded.

That makes sense :-)

> vim-enhanced and vim-X11 report dangling symlinks, but this becomes a normal 
> symlink when the required vim-common is installed.

OK

> vim -minimal has no documentation, I'd like to keep it this way. Maybe I'll add
> the license, but nothing more.

The License file would be nice.

Just two small things:
- vim-X11 BuildRequires libSM-devel, but that one is already required by libXt-devel
- Requires: /usr/bin/desktop-file-install on line 305, replace /usr/bin with %{_bindir}
Comment 8 Robert Scheck 2007-04-22 17:58:37 EDT
Karsten, why are you shipping man pages in UTF-8 AND in ISO8859-X/KOI8-R. IMHO 
this doesn't make sense, UTF-8 should be enough and the stuff should put be in 
the man pages directory without any .CHARSET extension.
Comment 9 Patrice Dumas 2007-04-22 18:07:18 EDT
(In reply to comment #8)
> Karsten, why are you shipping man pages in UTF-8 AND in ISO8859-X/KOI8-R. IMHO 
> this doesn't make sense, UTF-8 should be enough and the stuff should put be in 
> the man pages directory without any .CHARSET extension.

It does make sense. For those who use a non UTF8 encoded 
locale. I don't know exactly how man select the man page 
based on the locale encoding, maybe it doesn't really work 
right, but if it is broken man should be fixed, not the man 
pages.
Comment 10 Robert Scheck 2007-04-22 18:11:39 EDT
Well...UTF-8 is the Fedora default charset, anything else is normally not 
likely seen. But finally, please put the UTF-8 to the man directory itself
and not into .UTF-8 directory as UTF-8 is really the Fedora default charset.
Comment 11 Robert Scheck 2007-04-22 18:12:23 EDT
Nevertheless the abused directories have to be provided either by the man,
man-pages or the filesystem package! :)
Comment 12 Ruben Kerkhof 2007-06-17 03:04:00 EDT
Ping?
Comment 13 Ruben Kerkhof 2008-10-19 08:09:38 EDT
Review for release 1.fc10:
* RPM name is OK
* Source vim-7.2.tar.bz2 is the same as upstream
* Source vim-7.2-lang.tar.gz is the same as upstream
* Source vim-7.2-extra.tar.gz is the same as upstream
* Source forth.vim is the same as upstream
* This is the latest version
* Builds fine in mock
* rpmlint of vim-minimal looks OK
* rpmlint of vim-X11 looks OK
* rpmlint of vim-debuginfo looks OK
* rpmlint of vim-enhanced looks OK
* rpmlint of vim-common looks OK
* File list of vim-minimal looks OK
* File list of vim-X11 looks OK
* File list of vim-debuginfo looks OK
* File list of vim-enhanced looks OK
* File list of vim-common looks OK
* Config files of vim-minimal looks OK
* Config files of vim-enhanced looks OK
* Config files of vim-common looks OK

Needs work:
* Desktop file: the Categories tag should not contain Application any more
  (wiki: Packaging/Guidelines#desktop)
* Desktop file: the category Application is not valid
  (http://standards.freedesktop.org/menu-spec/latest/apa.html)
* As vim ships icons in the hicolor directory, it should have "Requires: hicolor-icon-theme"  https://www.redhat.com/archives/fedora-extras-list/2006-September/msg00282.html
* A few rpmlint warnings:
[ruben@slice vim]$ rpmlint vim-common-7.2.022-1.fc10.x86_64.rpm | grep spurious
vim-common.x86_64: W: spurious-executable-perm /usr/share/doc/vim-common-7.2.022/README_ami.txt.info
vim-common.x86_64: W: spurious-executable-perm /usr/share/doc/vim-common-7.2.022/README.txt.info
vim-common.x86_64: W: spurious-executable-perm /usr/share/doc/vim-common-7.2.022/README_amibin.txt.info
vim-common.x86_64: W: spurious-executable-perm /usr/share/doc/vim-common-7.2.022/README_amisrc.txt.info

* Like Robert said in comment #11, the man directories are not owned by any package, please resolve this.
Comment 14 Karsten Hopp 2008-10-20 11:33:40 EDT
I've built a new vim version in koji with fixes for those issues.
Please have a look at the packages in http://kojipkgs.fedoraproject.org/packages/vim/7.2.025/1.fc10/
Comment 15 Ruben Kerkhof 2008-10-20 14:46:06 EDT
Thanks Karsten,

You've commented out the line below, probably accidental?
%clean
#rm -rf $RPM_BUILD_ROOT

Furthermore all issues have been resolved, package is APPROVED.
Thanks for your help.
Comment 16 Karsten Hopp 2008-10-21 06:38:10 EDT
yes, I forgot to re-enable that. Thanks for catching that one, I'll build a fixed package.

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