[Bug 1201338] Review Request: python-uniseg - A pure Python module to determine Unicode text segmentation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



--- Comment #1 from Michael Schwendt (Fedora Packager Sponsors Group) <bugs.michael@xxxxxxx> ---
[not a full review - just some drive-by comments]

 * https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
 * https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
 * https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


> Version:        0.7.1

The included README says 0.7.0, so in case you've packaged a pre-release
snapshot, it should either follow the pre-release versioning guidelines when
keeping "Version: 0.7.1" or switch to "Version: 0.7.0" and follow the
post-release guidelines:

 * https://fedoraproject.org/wiki/Packaging:Guidelines#Version_and_Release


> %files
> %{python2_sitelib}/*egg-info
> %{python2_sitelib}/uniseg
> %defattr(0755,root,root,-)
> %{_bindir}/uniseg-dbpath
> %{python2_sitelib}/uniseg/wraptest.py
> %{python2_sitelib}/uniseg/samples/uniwrap.py
> %{python2_sitelib}/uniseg/samples/unibreak.py
> %{python2_sitelib}/uniseg/samples/wxwrapdemo.py
> %{python2_sitelib}/uniseg/sentencebreaktest.py
> %{python2_sitelib}/uniseg/graphemeclustertest.py
> %{python2_sitelib}/uniseg/wordbreaktest.py

This %files section looks much as if it is the result of trial-and-error build
attempts:

> %{python2_sitelib}/uniseg
> ...
> %{python2_sitelib}/uniseg/wraptest.py

If  %{python2_sitelib}/uniseg  is a directory, the line

  %{python2_sitelib}/uniseg

includes the directory and everything (anything!) in it. That is, all the
following lines lines in the %files section, which specify individual .py
files, are redundant. You also get warnings by rpmbuild: "warning: File listed
twice": https://kojipkgs.fedoraproject.org//work/tasks/20/9210020/build.log

Many packagers add a trailing slash to directory entries in %files sections to
make them more explicit, more readable, e.g.

  %{python2_sitelib}/uniseg/

does the same thing as

  %{python2_sitelib}/uniseg

but makes it more clear to the reader that it is supposed to be a directory.

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

The tool "rpmls" and "rpm" (rpm -qlvp …) are your friends when examining
package contentes / files lists.


> %if 0%{?with_python3}
> %files -n python3-uniseg
> %{python3_sitelib}/*egg-info
> %{python3_sitelib}/uniseg
> %defattr(0755,root,root,-)
> %{_bindir}/uniseg-dbpath
> %{python3_sitelib}/uniseg/wraptest.py
> %{python3_sitelib}/uniseg/samples/uniwrap.py
> %{python3_sitelib}/uniseg/samples/unibreak.py
> %{python3_sitelib}/uniseg/samples/wxwrapdemo.py
> %{python3_sitelib}/uniseg/sentencebreaktest.py
> %{python3_sitelib}/uniseg/graphemeclustertest.py
> %{python3_sitelib}/uniseg/wordbreaktest.py
> %endif # with_python3

Same here.


> # strip python3 from shebang line
> sed -i'' -e 's,^.*#!/usr/bin/python3.*$,#!/usr/bin/python,g' %{buildroot}%{_bindir}/uniseg-dbpath

The file is included in both packages, so in package python3-uniseg
/usr/bin/uniseg-dbpath would execute via /usr/bin/python which may be Python 2.


> BuildArch:      noarch
> BuildRequires:  sqlite

> Requires:       sqlite

Does it really need "sqlite" (no specific arch) at run-time?

  $ grep sqlite * -R
  Binary file uniseg/__pycache__/db.cpython-34.pyo matches
  Binary file uniseg/__pycache__/db.cpython-34.pyc matches
  Binary file uniseg/ucd.sqlite3 matches
  uniseg/db.py:import sqlite3
  uniseg/db.py:    dbname = 'ucd.sqlite3'
  uniseg/db.py:    _conn = sqlite3.connect(_dbpath)

Typically, a comment should explain such explicit Requires:
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

[...]

Consider pointing the fedora-review tool at review tickets like this:
fedora-review -b 1201338

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]