Bug 1919688 - Review Request: voikko-fi - A description of Finnish morphology written for libvoikko
Summary: Review Request: voikko-fi - A description of Finnish morphology written for l...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Miro Hrončok
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1764327
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-01-24 18:07 UTC by Ville-Pekka Vainio
Modified: 2021-02-06 21:29 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2021-02-06 21:29:48 UTC
Type: ---
Embargoed:
mhroncok: fedora-review+


Attachments (Terms of Use)

Description Ville-Pekka Vainio 2021-01-24 18:07:32 UTC
Spec URL: https://vpv.fedorapeople.org/packages/voikko-fi.spec
SRPM URL: https://vpv.fedorapeople.org/packages/voikko-fi-2.4-1.fc33.src.rpm
Description: A description of Finnish morphology written for libvoikko
Fedora Account System Username: vpv

This is a re-review request for a package rename. This package replaces the malaga-suomi-voikko package.

Please note: this will not build without libvoikko 4.3. I have not yet pushed libvoikko 4.3 to Rawhide, because pushing it without this package would break Finnish spell-checking. These two package are interdependent, although I've avoided circular dependencies in the spec files. Once someone commits to doing a review of this package, I will push libvoikko 4.3 to Rawhide.

Comment 1 Ville-Pekka Vainio 2021-01-24 18:30:18 UTC
I have a Copr repo with libvoikko 4.3 and voikko-fi 2.4 built here: https://copr.fedorainfracloud.org/coprs/vpv/Voikko-4.0/

Comment 2 Miro Hrončok 2021-01-24 22:32:08 UTC
Spec sanity:

1. BuildRequires:  python3 -> please BR python3-devel for Python packages, even if it is technically not needed.


2. Please avoid <= in obsoletes, it is confusing, since no package is actually versioned 1.19-20, it is usually 1.19-20.fc34 or similar. Use:

# This package replaces malaga-suomi-voikko
Provides:       malaga-suomi-voikko = %{version}-%{release}
Obsoletes:      malaga-suomi-voikko < 1.19-21


3. Using /usr/lib/voikko is fine, as recommended by Zbyszek, but in the specfile, please use macros, in this case, it is %{_prefix}/lib/voikko.


4. %doc ChangeLog CONTRIBUTORS COPYING README.md

Please move COPYING to %license instead of %doc. See https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text


5. Listing the entire /usr/lib/voikko directory in %files seems odd to me. Should that directory not be owned by libvoikko?

Comment 3 Ville-Pekka Vainio 2021-01-30 09:35:29 UTC
Updated spec: https://vpv.fedorapeople.org/packages/voikko-fi.spec
Updated SRPM: https://vpv.fedorapeople.org/packages/voikko-fi-2.4-2.fc33.src.rpm

(In reply to Miro Hrončok from comment #2)
> 1. BuildRequires:  python3 -> please BR python3-devel for Python packages,
> even if it is technically not needed.

This package is a bit different. It is not a Python package, it just uses a Python script to build the binary data files from the sources.
Python is used like this:
 python3 vvfst/generate_taivutuskaavat.py   --destdir=vvfst
 python3 vvfst/generate_lex.py  --destdir=vvfst
etc.

I believe I don't need to require python3-devel for running these scripts, or do I?

> 2. Please avoid <= in obsoletes, it is confusing, since no package is
> actually versioned 1.19-20, it is usually 1.19-20.fc34 or similar. Use:

> # This package replaces malaga-suomi-voikko
> Provides:       malaga-suomi-voikko = %{version}-%{release}
> Obsoletes:      malaga-suomi-voikko < 1.19-21

Ok. The current version in Rawhide is 1.19-13. I'd like to keep the Obsoletes version in 1.19-20 just because it's a nice round number. Is that ok?
 
> 3. Using /usr/lib/voikko is fine, as recommended by Zbyszek, but in the
> specfile, please use macros, in this case, it is %{_prefix}/lib/voikko.

Fixed.

> 4. %doc ChangeLog CONTRIBUTORS COPYING README.md
> 
> Please move COPYING to %license instead of %doc. See
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> LicensingGuidelines/#_license_text

Fixed.

> 5. Listing the entire /usr/lib/voikko directory in %files seems odd to me.
> Should that directory not be owned by libvoikko?

I guess it's a matter of taste. Technically one could use the library without that specific directory if one set the data file location via an environment variable.
If you think it's cleaner to have the directory owned by libvoikko, I can do that. I have updated my local copy of libvoikko.spec accordingly.

Comment 4 Miro Hrončok 2021-01-30 11:26:23 UTC
(In reply to Ville-Pekka Vainio from comment #3)
> > 1. BuildRequires:  python3 -> please BR python3-devel for Python packages,
> > even if it is technically not needed.
> 
> This package is a bit different. It is not a Python package, it just uses a
> Python script to build the binary data files from the sources.
> Python is used like this:
>  python3 vvfst/generate_taivutuskaavat.py   --destdir=vvfst
>  python3 vvfst/generate_lex.py  --destdir=vvfst
> etc.
> 
> I believe I don't need to require python3-devel for running these scripts,
> or do I?

You don technically need python3-devel (as I've tried to already say in my initial comment), but it makes our (Python maintainers) queries easier.
However, if you are strictly against this, I won't fight you.




> > 2. Please avoid <= in obsoletes, it is confusing, since no package is
> > actually versioned 1.19-20, it is usually 1.19-20.fc34 or similar. Use:
> 
> > # This package replaces malaga-suomi-voikko
> > Provides:       malaga-suomi-voikko = %{version}-%{release}
> > Obsoletes:      malaga-suomi-voikko < 1.19-21
> 
> Ok. The current version in Rawhide is 1.19-13. I'd like to keep the
> Obsoletes version in 1.19-20 just because it's a nice round number. Is that
> ok?

Totally OK. Also gives you some room for changes in F33.



> > 5. Listing the entire /usr/lib/voikko directory in %files seems odd to me.
> > Should that directory not be owned by libvoikko?
> 
> I guess it's a matter of taste. Technically one could use the library
> without that specific directory if one set the data file location via an
> environment variable.
> If you think it's cleaner to have the directory owned by libvoikko, I can do
> that. I have updated my local copy of libvoikko.spec accordingly.

Are there (even theoretically) other packages that will install into that directory? If yes, I'd transfer the ownership to the lib.

Comment 5 Miro Hrončok 2021-01-30 11:55:00 UTC
Also, what does the 5 stand for in /usr/lib/voikko/5 ? And what does mor-standard stand for in /usr/lib/voikko/5/mor-standard ?

I.e. does "5" belong here or to the library? I honestly don't know, because I lack domain knowledge.

Comment 6 Ville-Pekka Vainio 2021-01-30 19:28:39 UTC
Updated spec: https://vpv.fedorapeople.org/packages/voikko-fi.spec
Updated SRPM: https://vpv.fedorapeople.org/packages/voikko-fi-2.4-3.fc33.src.rpm

(In reply to Miro Hrončok from comment #4)
> You don technically need python3-devel (as I've tried to already say in my
> initial comment), but it makes our (Python maintainers) queries easier.
> However, if you are strictly against this, I won't fight you.

In that case I'm willing to BuildRequire python3-devel.
 
> Are there (even theoretically) other packages that will install into that
> directory? If yes, I'd transfer the ownership to the lib.

I'll aim to answer all of your questions here.

Yes, there could be other packages installing into that directory, at least in theory.

The number 5 in /usr/lib/voikko/5 is a dictionary format ID. It means it's the VFST (Varissuo Finite State Transducer) format, which is apparently a custom format developed by the Voikko project.
As this package (voikko-fi) implements format 5, the directory named 5 should definitely be owned by this package.

There is also format 3, HFST (Helsinki Finite State Transducer), which is developed by the University of Helsinki. Those data files would go under the directory named 3 and could be provided by other packages.
The HFST format supports languages other than Finnish. Apparently at least North Sámi, Lule Sámi and South Sámi are supported, but I'm not yet familiar with these options.
Supporting the HFST format requires hfst-ospell as a libvoikko dependency and that package is not yet in Fedora. I might add HFST support and maybe even some language files in the future, but I'd need to ask on the upstream mailing lists if anyone would actually use them on Fedora. I also don't yet know whether the licences of the language data for the different Sámi languages allow them to be shipped in Fedora.

From what I understand, mor-standard means "standard Finnish morphology". You could for example make a special morphology for Sukija by running "make vvfst-sukija" and apparently that would go under mor-sukija. Sukija a solr plugin for indexing Finnish texts (https://github.com/ahomansikka/sukija), but it's not in Fedora. That directory would also be provided by this package, probably in a sub-package, if someone wanted to contribute that stuff.

By the way, do I still need to %dir in Fedora?

Comment 7 Miro Hrončok 2021-01-31 14:17:54 UTC
> By the way, do I still need to %dir in Fedora?

Only if you want to include the directory without the files in it.

---

Based on what you say, I suggest to have this in libvoko:

%install
mkdir -p %{buildroot}%{_prefix}/lib/voikko

%files
%dir %{_prefix}/lib/voikko


And here to have:

# Installing this package without libvoikko would be useless.
# This package also installs files into a directory owned by libvoikko
Requires:       libvoikko >= 4.3

%files
%{_prefix}/lib/voikko/5



Since this is what is basically here I will now proced with automated checks from Fedora Review.

Comment 8 Miro Hrončok 2021-01-31 15:21:30 UTC
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

Package approved.


===== MUST items =====

Generic:
[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.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[-]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 3 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[?]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[?]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: voikko-fi-2.4-3.fc34.noarch.rpm
          voikko-fi-2.4-3.fc34.src.rpm
voikko-fi.noarch: W: spelling-error %description -l en_US unweighted -> weighted, underweight, unlighted
voikko-fi.noarch: W: only-non-binary-in-usr-lib
voikko-fi.src: W: spelling-error Summary(en_US) libvoikko 
voikko-fi.src: W: spelling-error %description -l en_US libvoikko 
voikko-fi.src: W: spelling-error %description -l en_US unweighted -> weighted, underweight, unlighted
voikko-fi.src:52: E: hardcoded-library-path in %{_prefix}/lib/voikko
voikko-fi.src:58: E: hardcoded-library-path in %{_prefix}/lib/voikko/5
2 packages and 0 specfiles checked; 2 errors, 5 warnings.

The errors are known and the package does that on purpose.




Rpmlint (installed packages)
----------------------------
voikko-fi.noarch: W: spelling-error %description -l en_US unweighted -> weighted, underweight, unlighted
voikko-fi.noarch: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 2 warnings.



Source checksums
----------------
https://www.puimula.org/voikko-sources/voikko-fi/voikko-fi-2.4.tar.gz.asc :
  CHECKSUM(SHA256) this package     : a672d575ba3a74c9ee786bd7d46e52997a3d5f6d9e2a38252fb66ec28d21e577
  CHECKSUM(SHA256) upstream package : a672d575ba3a74c9ee786bd7d46e52997a3d5f6d9e2a38252fb66ec28d21e577
https://www.puimula.org/voikko-sources/voikko-fi/voikko-fi-2.4.tar.gz :
  CHECKSUM(SHA256) this package     : 320b2d4e428f6beba9d0ab0d775f8fbe150284fbbafaf3e5afaf02524cee28cc
  CHECKSUM(SHA256) upstream package : 320b2d4e428f6beba9d0ab0d775f8fbe150284fbbafaf3e5afaf02524cee28cc


Requires
--------
voikko-fi (rpmlib, GLIBC filtered):
    libvoikko



Provides
--------
voikko-fi:
    malaga-suomi-voikko
    voikko-fi

Comment 9 Ville-Pekka Vainio 2021-01-31 19:46:34 UTC
Thank you, Miro!

Current status:
New Git repo requested here: https://pagure.io/releng/fedora-scm-requests/issue/31970
I've made a private branch in Fedora's git for the upcoming libvoikko changes, they are here: https://src.fedoraproject.org/rpms/libvoikko/blob/private-vpv-libvoikko-4.3/f/libvoikko.spec

Once voikko-fi gets a repo, I will build libvoikko 4.3 on Rawhide, which then enables me to build voikko-fi on Rawhide.
When that is done, we should have working spell-checking support and I can then build the newest upstream release of libreoffice-voikko.

Comment 11 Mohan Boddu 2021-02-02 20:51:44 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/voikko-fi

Comment 12 Fedora Update System 2021-02-06 21:29:48 UTC
FEDORA-2021-c69b42f6e6 has been pushed to the Fedora 34 stable repository.
If problem still persists, please make note of it in this bug report.


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