Bug 526122

Summary: Review Request: vim-latex - Tools to view, edit and compile LaTeX documents in Vim
Product: [Fedora] Fedora Reporter: Susi Lehtola <susi.lehtola>
Component: Package ReviewAssignee: Thomas Spura <tomspur>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting, reneploetz, tomspur
Target Milestone: ---Flags: tomspur: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 1.5-3.20091002.r1074.fc11 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-11-04 12:09:06 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Susi Lehtola 2009-09-28 21:06:04 UTC
Spec URL:
http://theory.physics.helsinki.fi/~jzlehtol/rpms/vim-latex.spec

SRPM URL:
http://theory.physics.helsinki.fi/~jzlehtol/rpms/vim-latex-1.5-1.20090901.r1069.fc11.src.rpm

Upstream URL: http://vim-latex.sourceforge.net/

Description:
A comprehensive set of tools to view, edit and compile LaTeX documents without
needing to ever quit Vim. Together, they provide tools starting from macros to
speed up editing LaTeX documents to compiling tex files to forward searching
.dvi documents.

rpmlint output:
vim-latex.noarch: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

Comment 1 Rene Ploetz 2009-09-30 20:37:39 UTC
This is an unofficial review (as I'm not sponsored yet):

+: ok
!: needs to be fixed
-: not applicable

MUST Items:
[+] rpmlint comes out clean (the no-documentation warning is not a problem when having a *-doc package, but see below)
[+] packages are named according to package guidelines
[+] spec file name matches base package name
[+] the package license (Vim charityware) is correct and allowed in Fedora (the license is GPL-compatible)
[+] license field matches the actual license
[-] license packaged in %doc if available separately in original package
[+] the spec file is legible and written in American English
[+] package md5sum matches upstream (3f0e34465b577aac6448c9c95da71abf)
[+] package builds fine in koji
[-] locales are properly handled
[+] no system libraries are bundled
[-] if package installs libraries in default paths run ldconfig in
%post/%postun
[+] package owns the directories it creates
[+] no file is listed twice
[+] permissions on files are explicitly set (via defattr)
[+] package must contain %clean with rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
[+] macros are consistently used
[+] package does contain code and/or permissible content
[+] (large) documentation goes into *-doc subpackage (see comments below)
[-] header files must be included in a *-devel subpackage
[-] static files must be included in a *-static subpackage
[-] packaging pkgconfig(.pc) files requires to set "Requires: pkgconfig"
[-] library files without a suffix (foo.so) must go into -devel subpackage if
libraries with a suffix (e.g. foo.so.0.0) are present.
[-] %{name}-devel packages must specify a fully versioned dependency on the
%{name} package
[-] packages must not contain any libtool (.la) archives
[-] (most) GUI applications need to include a %{name}.desktop file
[+] packages must not own any file or directory already owned by another package
[+] first command in %install must be rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
[+] all filenames in package are valid UTF-8


Should Items:
[!] if source package does not include license text as separate file, packager
should query upstream to include it
[-] if available,  description and summary in spec file should contain
translations for non-English languages
[+] packages build fine in mock
[+] packages should compile on all supported architectures
[+] packages work as expected in a short test
[+] scriptlets - if used - must be sane
[-] non-devel subpackages should require the base package
[-] pkgconfig(.pc) files should be placed in -devel package
[-] if package does require a file outside of /etc, /bin, /sbin, /usr/bin, or
/usr/sbin, packager should require the package which provides the file (not the
file alone)


I have only a singe point to comment on:
Upstream does not include any kind of license text as separate file. Did you try to query them to include a LICENSE file in their next release?
This is by no means a requirement, but it would be better to verify that our package is legal even their homepage somehow vanishes and we have to prove our the license classification.

Comment 2 Thomas Spura 2009-10-08 10:42:09 UTC
This is a nice program, writing and compiling is ok, but as described in the doc \lv for viewing the .dvi file does *not* work here.

Can't see why atm. Does this work for you?
evince can't open the dvi file either, so I'd suggest to patch vim-latex for using pdf or ps as output format.

What do you think, Jussi?

Comment 3 Susi Lehtola 2009-10-08 11:22:58 UTC
I haven't tried it myself- I packaged this just because there have been some requests on fedora-list.

Anyway, it's fixed now. I added the missing Requires: tex(latex) and xdvi.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/vim-latex.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/vim-latex-1.5-2.20090901.r1069.fc11.src.rpm

Comment 4 Thomas Spura 2009-10-08 19:22:04 UTC
This is my first official review and I run into some questions, but at least a good start…

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: 
       [] devel/i386 
       [] devel/x86_64
       [] F11/i386 
       [x] F11/x86_64
 [x] Rpmlint output:
     Source RPM:
$ rpmlint ../SRPMS/vim-latex-1.5-2.20090901.r1069.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
     Binary RPM(s):
rpmlint ../RPMS/noarch/vim-latex-doc-1.5-2.20090901.r1069.fc11.noarch.rpm ../RPMS/noarch/vim-latex-1.5-2.20090901.r1069.fc11.noarch.rpm 
vim-latex.noarch: W: no-documentation
2 packages and 0 specfiles checked; 0 errors, 1 warnings.

Documentation is in a extra package, so this is ok.

 [!] Buildroot is correct
     (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type: Vim
 [-] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     Upstream source: 3f0e34465b577aac6448c9c95da71abf
     Build source:    3f0e34465b577aac6448c9c95da71abf
 [-] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [!] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.
 [x] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [!] Latest version is packaged.
 [!] Package does not include license text files separate from upstream.
 [x] Package functions as described (no hardware to test with).
 [x] Scriptlets must be sane, if used.
 [x] The placement of pkgconfig(.pc) files is correct.

Issues/Summary:
* rpmlint would be quiet, if the licence would be %doc.
* Buildroot is not the standard one, so please use the one mentioned above or delete it completely, because it's not needed anymore.
______
Suggestions:
* there is a newer version, but that can be done later as an update.
* As mentioned in comment #1 no licence file is supported, so please query upstream to include one in the future.
______
my problems: ;)
* are the permissions correctly set with 'cp -a'? cp -a preserves all timestamps and permissions if they are correctly set in the sources. In this case, they are. But does this count as 'properly set'?

Comment 5 Susi Lehtola 2009-10-08 20:23:49 UTC
(In reply to comment #4)
>  [!] Buildroot is correct
>      (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))

See
 http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
it is the second one from the top, and is a valid option. (I used vim's rpm spec file template, which uses that option.)

>  [!] Permissions on files are set properly.

This refers to file permissions: the owner, group and the file mask must be correct.
http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions

There are no executables, and all files are owned by root:root with 644 permissions. So this is OK.

>  [!] Latest version is packaged.
>  [!] Package does not include license text files separate from upstream.

> my problems: ;)
> * are the permissions correctly set with 'cp -a'? cp -a preserves all
> timestamps and permissions if they are correctly set in the sources. In this
> case, they are. But does this count as 'properly set'?  

Yes, it does.

**

New release at
http://theory.physics.helsinki.fi/~jzlehtol/rpms/vim-latex.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/vim-latex-1.5-2.20091002.r1074.fc11.src.rpm

Comment 6 Susi Lehtola 2009-10-08 20:26:03 UTC
Assign the bug to yourself and change the status to ASSIGNED.

Comment 7 Thomas Spura 2009-10-08 22:39:37 UTC
md5sum from new sources:
   Upstream: 59420b4e4e13d8ea92806b4179b6311b
   Packaged: 59420b4e4e13d8ea92806b4179b6311b

rpmlint didn't chance…

The problem with the permissions were, what if upstream is lazy with the executable bit... I know some projects, who don't care about this, but here it's ok. (e.g. the hiding outline.py in /ftplugin/latex-suite/, in the provided Makefile it's double checked.)

latextags ltags are not installed. Is this by intention?
They are not needed necessarily, so OK anyway.

Why are you installing the folder 'latex-suite' twice?
%{_datadir}/vim/vimfiles/ftplugin/latex-suite/
%{_datadir}/vim/vimfiles/latex-suite/

%{_datadir}/vim/vimfiles/latex-suite/ can be removed, isn't it?

Viewing does not work anymore with the new release. evince is opened instead of xdvi. Don't know why.

Comment 8 Thomas Spura 2009-10-08 22:56:49 UTC
Viewing works now. I just had to adjust the default values in nautilus, when opening a dvi file…


For the other small issues would a 'make install' instead of your manual installation be the best solution…

Comment 9 Susi Lehtola 2009-10-08 23:23:47 UTC
Hmm, I wonder why I hadn't seen that there was a make install option available.. Fixed.

Now there are a couple of files in %{_bindir}, and rpmlint is clean.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/vim-latex.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/vim-latex-1.5-3.20091002.r1074.fc11.src.rpm

Comment 10 Thomas Spura 2009-10-08 23:58:28 UTC
Why is /vim/vimfiles/doc/imaps.txt in files, and not in %doc?

Isn't it the same as latex*.txt?

Comment 11 Susi Lehtola 2009-10-09 10:41:36 UTC
(In reply to comment #10)
> Why is /vim/vimfiles/doc/imaps.txt in files, and not in %doc?
> 
> Isn't it the same as latex*.txt?  

Well, that's an aesthetical bug - it doesn't affect the rpm in any meaningful way, other than marking the file as documentation in rpm's internal bookkeeping... Fixed in the next release. Any more comments?

Comment 12 Thomas Spura 2009-10-09 12:04:27 UTC
Just one last:

APPROVED

Comment 13 Susi Lehtola 2009-10-09 13:38:23 UTC
Thanks for the review!

New Package CVS Request
=======================
Package Name: vim-latex
Short Description: Tools to view, edit and compile LaTeX documents in Vim
Owners: jussilehtola
Branches: F-10 F-11 F-12
InitialCC:

Comment 14 Kevin Fenzi 2009-10-10 22:13:16 UTC
cvs done.

Comment 15 Fedora Update System 2009-10-10 23:47:03 UTC
vim-latex-1.5-3.20091002.r1074.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/vim-latex-1.5-3.20091002.r1074.fc10

Comment 16 Fedora Update System 2009-10-10 23:47:09 UTC
vim-latex-1.5-3.20091002.r1074.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/vim-latex-1.5-3.20091002.r1074.fc11

Comment 17 Fedora Update System 2009-10-14 01:38:09 UTC
vim-latex-1.5-3.20091002.r1074.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update vim-latex'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-10420

Comment 18 Fedora Update System 2009-10-14 01:53:17 UTC
vim-latex-1.5-3.20091002.r1074.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update vim-latex'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-10469

Comment 19 Fedora Update System 2009-11-04 12:08:59 UTC
vim-latex-1.5-3.20091002.r1074.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 20 Fedora Update System 2009-11-04 12:31:13 UTC
vim-latex-1.5-3.20091002.r1074.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.