[Bug 226526] New: Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

Summary: Merge Review: vim
           Product: Fedora Extras
           Version: devel
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: normal
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail]

Fedora Merge Review: vim

http://cvs.fedora.redhat.com/viewcvs/devel/vim/
Initial Owner: [e-mail]

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

------- Additional Comments From [e-mail]  2007-02-06 07:18 EST -------
vim-7.0.191-1.fc7 has lots of spec file fixes, please don't review older releases

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

------- Additional Comments From [e-mail]  2007-02-18 17:42 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

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

------- Additional Comments From [e-mail]  2007-02-21 10:52 EST -------
fixed in vim-7.0.195-2.fc7

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO
               Flag|                            |needinfo?(nobody@fedoraproje
                   |                            |ct.org)

------- Additional Comments From [e-mail]  2007-03-13 11:37 EST -------
What's the status of this review ? Approved ?

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |ASSIGNED
         AssignedTo|[e-mail]    |[e-mail]
               Flag|                            |fedora-review?

------- Additional Comments From [e-mail]  2007-03-18 08:11 EST -------
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.

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |medium

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo?(nobody@fedoraproje|
                   |ct.org)                     |

------- Additional Comments From [e-mail]  2007-04-16 11:30 EST -------
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.

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

------- Additional Comments From [e-mail]  2007-04-17 17:11 EST -------
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}

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|normal                      |medium

------- Additional Comments From [e-mail]  2007-04-22 17:58 EST -------
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.

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

------- Additional Comments From [e-mail]  2007-04-22 18:07 EST -------
(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.

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

------- Additional Comments From [e-mail]  2007-04-22 18:11 EST -------
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.

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

------- Additional Comments From [e-mail]  2007-04-22 18:12 EST -------
Nevertheless the abused directories have to be provided either by the man,
man-pages or the filesystem package! :)

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: vim

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226526

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEEDINFO

------- Additional Comments From [e-mail]  2007-06-17 03:04 EST -------
Ping?

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226526

--- Comment #13 from 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.h...
* 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.

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226526

Karsten Hopp  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo?([e-mail] |
                   |m)                          |

--- Comment #14 from 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/

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226526

Ruben Kerkhof  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

--- Comment #15 from 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.

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226526

--- Comment #16 from 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.

[Bug 226526] Merge Review: vim

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226526

Ruben Kerkhof  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |RAWHIDE