[Bug 1919688] New: Review Request: voikko-fi - A description of Finnish morphology written for libvoikko
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
Bug ID: 1919688 Summary: Review Request: voikko-fi - A description of Finnish morphology written for libvoikko Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: vpvainio@iki.fi QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
Ville-Pekka Vainio vpvainio@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value
--- Comment #1 from Ville-Pekka Vainio vpvainio@iki.fi --- 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/
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mhroncok@redhat.com Depends On| |1764327
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1764327 [Bug 1764327] libvoikko-4.3 is available
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
--- Comment #2 from Miro Hrončok mhroncok@redhat.com --- 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/LicensingGuideline...
5. Listing the entire /usr/lib/voikko directory in %files seems odd to me. Should that directory not be owned by libvoikko?
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
--- Comment #3 from Ville-Pekka Vainio vpvainio@iki.fi --- 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)
- 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?
- 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?
- 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.
- %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.
- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
--- Comment #4 from Miro Hrončok mhroncok@redhat.com --- (In reply to Ville-Pekka Vainio from comment #3)
- 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.
- 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.
- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
--- Comment #5 from Miro Hrončok mhroncok@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
--- Comment #6 from Ville-Pekka Vainio vpvainio@iki.fi --- 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?
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |mhroncok@redhat.com Flags| |fedora-review?
--- Comment #7 from Miro Hrončok mhroncok@redhat.com ---
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #8 from Miro Hrončok mhroncok@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
--- Comment #9 from Ville-Pekka Vainio vpvainio@iki.fi --- 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/...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
--- Comment #10 from Miro Hrončok mhroncok@redhat.com --- See also https://fedoraproject.org/wiki/Package_update_HOWTO#Multiple_Packages
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
--- Comment #11 from Mohan Boddu mboddu@bhujji.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/voikko-fi
https://bugzilla.redhat.com/show_bug.cgi?id=1919688 Bug 1919688 depends on bug 1764327, which changed state.
Bug 1764327 Summary: libvoikko-4.3 is available https://bugzilla.redhat.com/show_bug.cgi?id=1764327
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |ERRATA
https://bugzilla.redhat.com/show_bug.cgi?id=1919688
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |ERRATA Last Closed| |2021-02-06 21:29:48
--- Comment #12 from Fedora Update System updates@fedoraproject.org --- 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.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com