Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226526 - Merge Review: vim
Summary: Merge Review: vim
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ruben Kerkhof
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 21:15 UTC by Nobody's working on this, feel free to take it
Modified: 2008-11-01 23:45 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-11-01 23:45:15 UTC
ruben: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 21:15:12 UTC
Fedora Merge Review: vim

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

Comment 1 Karsten Hopp 2007-02-06 12:18:45 UTC
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 22:42:19 UTC
/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 15:52:57 UTC
fixed in vim-7.0.195-2.fc7

Comment 4 Karsten Hopp 2007-03-13 15:37:21 UTC
What's the status of this review ? Approved ?

Comment 5 Ruben Kerkhof 2007-03-18 12:11:35 UTC
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 15:30:28 UTC
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 21:11:11 UTC
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 21:58:37 UTC
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 22:07:18 UTC
(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 22:11:39 UTC
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 22:12:23 UTC
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 07:04:00 UTC
Ping?

Comment 13 Ruben Kerkhof 2008-10-19 12:09:38 UTC
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 15:33:40 UTC
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 18:46:06 UTC
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 10:38:10 UTC
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.