Bug 1223623 - Review Request: python-num2words - Modules to convert numbers to words
Summary: Review Request: python-num2words - Modules to convert numbers to words
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-05-21 04:00 UTC by William Moreno
Modified: 2015-06-22 10:36 UTC (History)
4 users (show)

Fixed In Version: python-num2words-0.5.3-1.fc22
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-06-22 10:34:10 UTC
Type: ---
Embargoed:
zbyszek: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description William Moreno 2015-05-21 04:00:18 UTC
Spec URL: https://williamjmorenor.fedorapeople.org/rpmdev/python-num2words.spec
SRPM URL: https://williamjmorenor.fedorapeople.org/rpmdev/python-num2words-0.5.2-1.fc21.src.rpm
Description: Modules to convert numbers to words
Fedora Account System Username: williamjmorenor

Comment 1 Raphael Groner 2015-05-25 08:49:11 UTC
Some informal things to mention:

Do not add any license text by your own. Upstream provides it.
https://github.com/savoirfairelinux/num2words/blob/master/COPYING

Please remove:
Source1:        https://www.gnu.org/licenses/lgpl-2.1.txt
install -p -m 644 %{SOURCE1} \
%{_builddir}/%{pypi_name}-%{version}/LICENSE


> %exclude %{python2_sitelib}/tests/
Please patch setup.py file to not install tests at all, or remove it in %install .


Does this work with Python3? If yes (and it seems so), you have to provide a build also for Python3.
https://fedoraproject.org/wiki/Packaging:Python#Python_Version_Support

Comment 2 William Moreno 2015-05-29 14:36:54 UTC
Spec URL: https://williamjmorenor.fedorapeople.org/rpmdev/python-num2words.spec
SRPM URL: https://williamjmorenor.fedorapeople.org/rpmdev/python-num2words-0.5.2-2.fc21.src.rpm

-----
- 0.5.2-2
- Remove /test after %%check

I do not agree about remove the test of the gpl, I am packaging the tarball from pypi and this tarball do not include the text of the GPL.

The GPL requires than a copy of the license must be provived with the source, there is a mistake to not include the license file when you use GPL.

I have opened a issue on github to include the license file in the pypi tarball

https://github.com/savoirfairelinux/num2words/issues/21

Comment 3 Zbigniew Jędrzejewski-Szmek 2015-05-29 18:29:12 UTC
Is there a particular reason to remove the tests? It's good to have tests in the installed package too.

Raphael is right: you should not download the license file from gnu.org. But you can use the file from the upstream repository: https://github.com/savoirfairelinux/num2words/blob/master/COPYING .

Looks OK otherwise. I'll take the review.

Comment 4 Zbigniew Jędrzejewski-Szmek 2015-05-29 18:39:13 UTC
See https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Valid_License_Short_Names:
LGPL should be changed to LGPLv2 or LGPLv2+.

Comment 5 William Moreno 2015-06-02 03:21:34 UTC
Spec URL: http://rmsconsultoresnicaragua.com/rpmdev/python-num2words.spec
SRPM URL: http://rmsconsultoresnicaragua.com/rpmdev/python-num2words-0.5.2-3.fc22.src.rpm

- 0.5.2-3
- Add suport for Pyhton3
- Use %%license file from Github
- Include test files in package

Comment 6 Zbigniew Jędrzejewski-Szmek 2015-06-02 13:32:19 UTC
Fedoras <= 20 are EOL. %if 0%{?fedora} can be simplified to %if %{?fedora}.

There's no need to apply the license through a patch... Just put
Source1: https://raw.githubusercontent.com/savoirfairelinux/num2words/master/COPYING
...
%license %{SOURCE1}
(Not tested)

Please fix the license tag. See comment #c4.

Hm, it's good to install tests, but they CANNOT be installed into a non-namespaced directory: /usr/lib/python2.7/site-packages/tests. This should be something like /usr/lib/python2.7/site-packages/num2words/tests. This is basically an upstream error. It is actually better not to install tests than install them here.

Comment 7 Zbigniew Jędrzejewski-Szmek 2015-06-02 13:51:40 UTC
Ah, sorry, I didn't want to flip the flag yet.

Comment 8 Raphael Groner 2015-06-02 13:52:32 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
…
> Please fix the license tag. See comment #c4.
> 
> Hm, it's good to install tests, but they CANNOT be installed into a
> non-namespaced directory: /usr/lib/python2.7/site-packages/tests. This
> should be something like /usr/lib/python2.7/site-packages/num2words/tests.
> This is basically an upstream error. It is actually better not to install
> tests than install them here.

Maybe install the tests as development or documentation files. You could use:
%doc %{buildroot}%{python2_sitelib}/tests/
%exclude %{python2_sitelib}/tests/

%doc %{buildroot}%{python3_sitelib}/tests/
%exclude %{python3_sitelib}/tests/

Usage of %{buildroot} is necessary to get the files from installation location, otherwise %doc will fail with its search in the source folder.

Comment 9 William Moreno 2015-06-02 15:19:32 UTC
Spec URL: http://rmsconsultoresnicaragua.com/rpmdev/python-num2words.spec
SRPM URL: http://rmsconsultoresnicaragua.com/rpmdev/python-num2words-0.5.2-4.fc22.src.rpm


- 0.5.2-4
- Move test files under the %%{pypi_name} directory
- Include test files in a devel package
- Fix %%license tag

I have tried Raphael suggestion but the %doc macro was not working to put the test files under a different directory than %{python2_sitelib} and %{python3_sitelib}, but I like the idea to make the devel package. So after %%chech I move the the test files under the %%{pypi_name} and include this files the devel package.

I have many sources in my SOURCES dir and I have more than a COPIYIN and LICENSE files under it, this the reason why I am pathing the license text and not include it directly as Source1.

Comment 10 Raphael Groner 2015-06-02 15:46:20 UTC
(In reply to William Moreno from comment #9)
> I have tried Raphael suggestion but the %doc macro was not working to put
> the test files under a different directory than %{python2_sitelib} and
> %{python3_sitelib}, but I like the idea to make the devel package. So after
> %%chech I move the the test files under the %%{pypi_name} and include this
> files the devel package.

I doubt this is the right place to do that. The stuff in %check should not contain installation instructions, so better do either copy tests folder in %install or at least move it and symlink it for future optional %check execution. Please note there's the --nocheck option for rpmbuild to skip %check manually, though not used in mock or koji by default.

Comment 11 William Moreno 2015-06-02 16:41:31 UTC
I think than can be easier to %exclude test for the spec, packaging guidelines requires to run the test not to include test files in the rpm. 

But I will wait if Zbigniew have a suggestion about that.

Comment 12 Zbigniew Jędrzejewski-Szmek 2015-06-02 16:46:54 UTC
(In reply to William Moreno from comment #11)
> I think than can be easier to %exclude test for the spec, packaging
> guidelines requires to run the test not to include test files in the rpm. 
Yeah, I think this is probably the best solution. Normally it is good to include tests so users can run them easily, but if upstream packaging doesn't support that, than it's not worth the trouble.

Comment 13 Raphael Groner 2015-06-02 17:00:54 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> (In reply to William Moreno from comment #11)
> > I think than can be easier to %exclude test for the spec, packaging
> > guidelines requires to run the test not to include test files in the rpm. 
> Yeah, I think this is probably the best solution. Normally it is good to
> include tests so users can run them easily, but if upstream packaging
> doesn't support that, than it's not worth the trouble.

That's okay for me. It was just my suggestion to package those files into the right place and avoid any confusion about %exclude usage. You should discuss that further with upstream.

Comment 14 William Moreno 2015-06-02 17:46:40 UTC
Spec URL: http://rmsconsultoresnicaragua.com/rpmdev/python-num2words.spec
SRPM URL: http://rmsconsultoresnicaragua.com/rpmdev/python-num2words-0.5.2-5.fc22.src.rpm

- 0.5.2-5
- Exclude test files

Open a issue with upstream to move the test files to other place so I will be able to include these files in futures releases

https://github.com/savoirfairelinux/num2words/issues/22

Comment 15 Zbigniew Jędrzejewski-Szmek 2015-06-02 17:57:06 UTC
===== 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.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "LGPL (v2.1 or later)", "Unknown or generated". 2 files have
     unknown license. Detailed output of licensecheck in /var/tmp/1223623
     -python-num2words/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[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.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: 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 2 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).
[-]: 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]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[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 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

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

Rpmlint
-------
Checking: python-num2words-0.5.2-5.fc23.noarch.rpm
          python3-num2words-0.5.2-5.fc23.noarch.rpm
          python-num2words-0.5.2-5.fc23.src.rpm
python-num2words.noarch: E: incorrect-fsf-address /usr/share/licenses/python-num2words/COPYING
python3-num2words.noarch: E: incorrect-fsf-address /usr/share/licenses/python3-num2words/COPYING
3 packages and 0 specfiles checked; 2 errors, 0 warnings.

Upstream issue.

Rpmlint (installed packages)
----------------------------
python3-num2words.noarch: E: incorrect-fsf-address /usr/share/licenses/python3-num2words/COPYING
python-num2words.noarch: E: incorrect-fsf-address /usr/share/licenses/python-num2words/COPYING
2 packages and 0 specfiles checked; 2 errors, 0 warnings.

Suggestions:
1. Add the following to avoid creating duplicate directories for both subpackages:

%global _docdir_fmt %{name}

2. Replace `` and '' in the description with proper quotes “”. We're all on unicode after all.

Package is APPROVED.

Comment 16 William Moreno 2015-06-02 18:11:26 UTC
Thanks Zbigniew and Raphael for the comments and the review.

Zbigniew can you please set the fedora-review flag?

Comment 17 Zbigniew Jędrzejewski-Szmek 2015-06-02 18:12:17 UTC
It's set.

Comment 18 William Moreno 2015-06-02 18:17:26 UTC
New Package SCM Request
=======================
Package Name: python-num2words
Short Description: Modules to convert numbers to words
Upstream URL: https://github.com/savoirfairelinux/num2words
Owners: williamjmorenor
Branches: master f22 f21
InitialCC: williamjmorenor

Comment 19 Gwyn Ciesla 2015-06-03 20:28:08 UTC
Git done (by process-git-requests).

Comment 20 Fedora Update System 2015-06-03 21:06:27 UTC
python-num2words-0.5.2-6.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/python-num2words-0.5.2-6.fc21

Comment 21 Fedora Update System 2015-06-03 21:23:18 UTC
python-num2words-0.5.2-6.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/python-num2words-0.5.2-6.fc22

Comment 22 Fedora Update System 2015-06-05 23:37:49 UTC
python-num2words-0.5.2-6.fc22 has been pushed to the Fedora 22 testing repository.

Comment 23 Fedora Update System 2015-06-09 16:56:20 UTC
python-num2words-0.5.2-6.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-num2words-0.5.2-6.fc20

Comment 24 Fedora Update System 2015-06-09 19:37:20 UTC
python-num2words-0.5.3-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-num2words-0.5.3-1.fc20

Comment 25 Fedora Update System 2015-06-09 19:39:52 UTC
python-num2words-0.5.3-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/python-num2words-0.5.3-1.fc21

Comment 26 Fedora Update System 2015-06-09 19:40:35 UTC
python-num2words-0.5.3-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/python-num2words-0.5.3-1.fc22

Comment 27 Fedora Update System 2015-06-22 10:34:10 UTC
python-num2words-0.5.3-1.fc21 has been pushed to the Fedora 21 stable repository.

Comment 28 Fedora Update System 2015-06-22 10:34:44 UTC
python-num2words-0.5.3-1.fc20 has been pushed to the Fedora 20 stable repository.

Comment 29 Fedora Update System 2015-06-22 10:36:22 UTC
python-num2words-0.5.3-1.fc22 has been pushed to the Fedora 22 stable repository.


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