Bug 739331 - Rename Review: libreoffice-voikko - Finnish spellchecker and hyphenator extension for LibreOffice
Summary: Rename Review: libreoffice-voikko - Finnish spellchecker and hyphenator exten...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-09-17 19:20 UTC by Ville-Pekka Vainio
Modified: 2011-11-21 21:12 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-11-21 21:12:13 UTC
susi.lehtola: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Ville-Pekka Vainio 2011-09-17 19:20:54 UTC
Spec URL: http://vpv.fedorapeople.org/packages/libreoffice-voikko.spec
SRPM URL: http://vpv.fedorapeople.org/packages/libreoffice-voikko-3.2-1.fc15.src.rpm
Description: This package contains a Finnish spell-checking and hyphenation component for
LibreOffice. The actual spell-checking and hyphenation functionality is
provided by the Voikko library.

This is a re-review request for a package rename, the old package is called openoffice.org-voikko. Upstream renamed the package a while ago and since then Fedora has been shipping a somewhat outdated version with the previous name.

Here are the Provides and Obsoletes from the spec file:
Provides:         openoffice.org-voikko%{?_isa} = %{version}-%{release}
Obsoletes:        openoffice.org-voikko%{?_isa} < 3.1.2-5

The final openoffice.org-voikko release was 3.1.2-4.

Here are the rpmlint warnings:

libreoffice-voikko.i686: W: spelling-error Summary(en_US) hyphenator -> hyphenation, hyphenated, hyphenate
libreoffice-voikko.i686: E: binary-or-shlib-defines-rpath /usr/lib/libreoffice/share/extensions/voikko.uno.pkg/voikko.so ['$ORIGIN']
libreoffice-voikko.i686: E: zero-length /usr/lib/libreoffice/share/extensions/voikko.uno.pkg/SettingsDialog_en_US.default
libreoffice-voikko.src: W: spelling-error Summary(en_US) hyphenator -> hyphenation, hyphenated, hyphenate
libreoffice-voikko.src:12: W: macro-in-comment %{name}
libreoffice-voikko.src:12: W: macro-in-comment %{version}
3 packages and 0 specfiles checked; 2 errors, 4 warnings.

It is my understanding that the rpath is necessary. I could try confirming this from a LibreOffice package maintainer.

Comment 1 Susi Lehtola 2011-09-18 18:25:08 UTC
Since you seem to be actually compiling something, you need to build it in %build, and install in %install.

Comment 2 Ville-Pekka Vainio 2011-09-18 20:34:12 UTC
New SRPM: http://vpv.fedorapeople.org/packages/libreoffice-voikko-3.2-2.fc16.src.rpm

Changes:
- Build in the build section, install in the install section
- Add the _isa macro to all dependencies
- Update libvoikko dependencies to >= 3.0

I need to source %{libo_sdk}/setsdkenv_unix.sh in both %build and %install because the Makefile uses environment variables which get defined by that shell script.

Comment 3 Ville-Pekka Vainio 2011-09-18 20:59:04 UTC
I just noticed that the BuildRequires are broken, libo_version isn't used as a macro. I'll fix this soon, until then, please don't review this package.

Comment 4 Ville-Pekka Vainio 2011-09-25 15:50:16 UTC
New SRPM: http://vpv.fedorapeople.org/packages/libreoffice-voikko-3.2-3.fc15.src.rpm

The package should be good for review now.

Comment 5 Susi Lehtola 2011-09-25 17:34:47 UTC
rpmlint output:
libreoffice-voikko.src: W: spelling-error Summary(en_US) hyphenator -> hyphenation, hyphenate
libreoffice-voikko.src:12: W: macro-in-comment %{name}
libreoffice-voikko.src:12: W: macro-in-comment %{version}
libreoffice-voikko.x86_64: W: spelling-error Summary(en_US) hyphenator -> hyphenation, hyphenate
libreoffice-voikko.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/libreoffice/share/extensions/voikko.uno.pkg/voikko.so ['$ORIGIN']
libreoffice-voikko.x86_64: E: zero-length /usr/lib64/libreoffice/share/extensions/voikko.uno.pkg/SettingsDialog_en_US.default
3 packages and 0 specfiles checked; 2 errors, 4 warnings.


Please get rid of the rpath in voikko.so. The other are OK.


MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
$ md5sum libreoffice-voikko-3.2.tar.gz ../SOURCES/libreoffice-voikko-3.2.tar.gz
cd432ee55b424c38d841876db757098f  libreoffice-voikko-3.2.tar.gz
cd432ee55b424c38d841876db757098f  ../SOURCES/libreoffice-voikko-3.2.tar.gz

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. ~OK
- I see that an extra "-O" is passed to the compiler before Fedora optflags. 
  Please find out where it comes from and file bugs against libreoffice if 
  necessary.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned, architecture dependent dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
EPEL: Clean section exists. OK
EPEL: Buildroot cleaned before install. OK
EPEL: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK

Comment 6 Ville-Pekka Vainio 2011-09-25 18:36:28 UTC
The extra -O switch comes from the file /usr/lib64/libreoffice/basis3.4/sdk/settings/settings.mk which is included by libreoffice-voikko's Makefile. The full options that it passes are:

CC_FLAGS=-c -fpic -fvisibility=hidden
CC_FLAGS+=-O

I think the best way to solve this for now would be to pass "CC_FLAGS=-c -fpic -fvisibility=hidden" to the build in the spec file and maybe report a LibreOffice bug.

The rpath comes from the same file:
LIBRARY_LINK_FLAGS=-shared '-Wl,-rpath,$$ORIGIN'
COMP_LINK_FLAGS=$(LIBRARY_LINK_FLAGS)

I found the original review by Caolan and from reading this comment, I think the $ORIGIN rpath is necessary: https://bugzilla.redhat.com/show_bug.cgi?id=429550#c4

Comment 7 Susi Lehtola 2011-09-25 18:55:25 UTC
Very well. Please override CC_FLAGS in the spec file and file a bug against LibreOffice, IMHO the default CC_FLAGS should be set to %{optflags}.

The current provides and obsoletes are in order. Please ask the libreoffice people about the real necessity of the rpath.

However, I can't see any blockers so I declare this review

APPROVED


Please continue by requesting branches, and follow the instructions at
 http://fedoraproject.org/wiki/How_to_remove_a_package_at_end_of_life
for openoffice.org-voikko for F16 and rawhide.

Comment 8 Ville-Pekka Vainio 2011-09-26 14:01:25 UTC
About the rpath, Caolan answered my email and he said "I think its necessary in the general case, in case
an extension includes a pile of libraries which link to each-other." However, libreoffice-voikko is just one .so file. I'll try to remove the rpath and see if libreoffice-voikko works without it.

Comment 9 Ville-Pekka Vainio 2011-10-02 19:08:59 UTC
New SRPM: http://vpv.fedorapeople.org/packages/libreoffice-voikko-3.2-4.fc16.src.rpm

I've removed the rpath and overridden CC_FLAGS to drop the extra -O switch. The extension works fine without the rpath. I'll ask the LibreOffice maintainers' opinion on whether to put this package into F16 too or just into rawhide.

Comment 10 Susi Lehtola 2011-10-02 19:21:03 UTC
.. but this is just a rename, isn't it?

IMHO you can put it straight away in F16.

Comment 11 Ville-Pekka Vainio 2011-10-03 15:49:11 UTC
(In reply to comment #10)
> .. but this is just a rename, isn't it?
> 
> IMHO you can put it straight away in F16.

It's also a version bump, though not that significant. The openoffice.org-voikko package is in the comps group for Finnish and also required by LO's Finnish language package. Thus, it gets installed by default at least when you do a Finnish DVD or network install. This makes the rename a bit more risky at this stage of the F16 process. Caolán recommended that I put this package into Rawhide first and see if any dependencies break. If not, then it'd be safer to put it into F16.

Comment 12 Ville-Pekka Vainio 2011-10-03 15:51:13 UTC
New Package SCM Request
=======================
Package Name: libreoffice-voikko
Short Description: Finnish spellchecker and hyphenator extension for LibreOffice
Owners: vpv
Branches: f16

Comment 13 Gwyn Ciesla 2011-10-03 17:05:14 UTC
Git done (by process-git-requests).

Comment 14 Ville-Pekka Vainio 2011-11-21 21:12:13 UTC
I have retired openoffice.org-voikko in devel and I've also posted a rel-eng ticket about it: https://fedorahosted.org/rel-eng/ticket/4978 . The ticket has not been answered, but I'll close this bug anyway.


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