Bug 1973680 - Review Request: python-stopit - Raise asynchronous exceptions in other threads and more
Summary: Review Request: python-stopit - Raise asynchronous exceptions in other thread...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ben Beasley
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: fedora-neuro, NeuroFedora 1821189
TreeView+ depends on / blocked
 
Reported: 2021-06-18 13:08 UTC by Aniket Pradhan
Modified: 2021-07-02 01:20 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-07-01 01:13:19 UTC
Type: ---
Embargoed:
code: fedora-review+


Attachments (Terms of Use)

Description Aniket Pradhan 2021-06-18 13:08:12 UTC
Spec URL: https://major.fedorapeople.org/python-stopit/python-stopit.spec
SRPM URL: https://major.fedorapeople.org/python-stopit/python-stopit-1.1.2-1.fc34.src.rpm

Description: This module provides:

* a function that raises an exception in another thread, including the main
thread.
* two context managers that may stop its inner block activity on timeout.
* two decorators that may stop its decorated callables on timeout.

Developed and tested with CPython 2.6, 2.7, 3.3, and 3.4 on MacOSX. Should work
on any OS (xBSD, Linux, Windows) except when explicitly mentioned.

Fedora Account System Username: major

Koji Scratch Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=70360458

Comment 1 Arthur Bols 2021-06-19 16:53:20 UTC
Note: This is a preliminary (unofficial) review!

Hello,

I'm not a packager, but like to help out. Please remember that this is a preliminary (unofficial) review.


"Rpmlint (installed packages)" failed, so I manually installed it and ran rpmlint:
     python3-stopit.noarch: W: spelling-error %description -l en_US callables -> callable, callable s, calculable
     python3-stopit.noarch: W: spelling-error %description -l en_US xBSD -> BSD, x BSD
     1 packages and 0 specfiles checked; 0 errors, 2 warnings.


Package Review
==============

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


===== ISSUES =====
- License
  The license on the sources is changed from GPLv3 to MIT, but the setup.py still contains GPLv3. 
  Because of this, the license in PKG-INFO is still GPLv3. See: https://github.com/glenfant/stopit/pull/23
  I'm not sure if the package license should be changed, but an official reviewer will probably reply on this.

- Description 
  The last paragraph (see below) is unnecessary, and can be removed.
     
     Developed and tested with CPython 2.6, 2.7, 3.3 and 3.4 on MacOSX. Should work
     on any OS (xBSD, Linux, Windows) except when explicitly mentioned.}

  A small nitpick, a list seems a bit out of place in a package description, you could maybe change it to the PyPi description?

     Raise asynchronous exceptions in other threads, control the timeout of blocks or
     callables with two context managers and two decorators.
  
- In the %prep section is find command, but it doesn't seem to do anything. Is this necessary or am I missing something?

     find . -type f -name "*.py" -exec sed -i '/^#![  ]*\/usr\/bin\/env.*$/ d' {} 2>/dev/null ';'

===== 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.
[!]: 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.
[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 30720 bytes in 1 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

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.
[?]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
[x]: Binary eggs must be removed in %prep

===== 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.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: 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.
[x]: %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]: 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: python3-stopit-1.1.2-1.fc35.noarch.rpm
          python-stopit-1.1.2-1.fc35.src.rpm
python3-stopit.noarch: W: spelling-error %description -l en_US callables -> callable, callable s, calculable
python3-stopit.noarch: W: spelling-error %description -l en_US xBSD -> BSD, x BSD
python-stopit.src: W: spelling-error %description -l en_US callables -> callable, callable s, calculable
python-stopit.src: W: spelling-error %description -l en_US xBSD -> BSD, x BSD
2 packages and 0 specfiles checked; 0 errors, 4 warnings.




Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Source checksums
----------------
https://github.com/glenfant/stopit/archive/1.1.2/stopit-1.1.2.tar.gz :
  CHECKSUM(SHA256) this package     : df619f61469765aad691bc3dcaf0d4bae1fd4611120bd4a74e72f568ee7d8424
  CHECKSUM(SHA256) upstream package : df619f61469765aad691bc3dcaf0d4bae1fd4611120bd4a74e72f568ee7d8424


Requires
--------
python3-stopit (rpmlib, GLIBC filtered):
    python(abi)



Provides
--------
python3-stopit:
    python-stopit
    python3-stopit
    python3.10-stopit
    python3.10dist(stopit)
    python3dist(stopit)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1973680
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, Generic, Python
Disabled plugins: Haskell, R, PHP, SugarActivity, Perl, Ocaml, C/C++, fonts, Java
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Comment 2 Aniket Pradhan 2021-06-20 09:04:32 UTC
Hey Arthur!

Thanks for the review and your suggestions are really helpful.

* I have fixed the license in setup.py using sed. Hopefully, upstream would patch the issue soon.
* rpmlint only reports spelling errors. I think it should be fine.
* Added the paragraph to the description. Since the description can span multiple lines and can be broken into paragraphs, I think the list should be fine.
* The find command is from a generic spec file that I keep. It basically removes the unnecessary shebangs from files. Although it might not be required here, I usually prefer it to remain in the spec so that no such shebangs come in place in case of an update.

Thanks for the preliminary review. Please do let me know if any other information is required. Also, in case you are interested in packaging, do check out our channel #fedora-neuro or https://neuro.fedoraproject.org/ as we could definitely use your help :D

Comment 3 Arthur Bols 2021-06-20 13:33:58 UTC
Note: This is a preliminary (unofficial) review!

Thank you for the update!

The description looks better now. The list seems a bit redundant now, but changing the '>' to '*' also looks better, so It's fine for me.

I think the package can be approved.


I'd love to help out, I'll stop by. :)

Comment 4 Ben Beasley 2021-06-20 17:53:21 UTC
Thanks for the preliminary review and helpful feedback, Arthur! I’m claiming the review and will go through the details within the next day or so (hopefully sooner).

The handling of the license situation looks good; the PR was the right thing to do, and both the LICENSE file and README.rst say MIT which supports GPLv3 in setup.py being a copy-paste error.

Comment 5 Rafael Jeffman 2021-06-21 12:31:15 UTC
I know Ben Beasley is going to do a review, but I was working an (another) preliminary unofficial review, and found nearly the same issues, but with some different tool results than Arhur.


Package Review
==============

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

===== ISSUES =====

- rpmlint is reporting an error:
    python3-stopit.noarch: E: summary-too-long C Timeout control decorator and context managers, raise any exception in another thread

- The issue with the license is slightly more than a copy-paste error, as it is related to a license change.

- Upstream was working on the license change in 2017/2018 and did not finish it, yet.

- Upstream code has not been tested with Python 3.7+, and as Python 3.10 is bringing more changes.

- Upstream code seems abandoned, is the packager willing to assume/fork the original source code and maintain it in the future?

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

Generic:
[!]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "Expat License", "*No copyright* Expat
     License". 7 files have unknown license. Detailed output of
     licensecheck in /home/rjeffman/1973680-python-stopit/licensecheck.txt
[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 30720 bytes in 1 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

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]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
[x]: Binary eggs must be removed in %prep

===== 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.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: 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.
[x]: %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]: 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: python3-stopit-1.1.2-1.fc35.noarch.rpm
          python-stopit-1.1.2-1.fc35.src.rpm
python3-stopit.noarch: E: summary-too-long C Timeout control decorator and context managers, raise any exception in another thread
python3-stopit.noarch: W: spelling-error %description -l en_US callables -> callable, callable s, calculable
python-stopit.src: E: summary-too-long C Timeout control decorator and context managers, raise any exception in another thread
python-stopit.src: W: spelling-error %description -l en_US callables -> callable, callable s, calculable
2 packages and 0 specfiles checked; 2 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Source checksums
----------------
https://github.com/glenfant/stopit/archive/1.1.2/stopit-1.1.2.tar.gz :
  CHECKSUM(SHA256) this package     : df619f61469765aad691bc3dcaf0d4bae1fd4611120bd4a74e72f568ee7d8424
  CHECKSUM(SHA256) upstream package : df619f61469765aad691bc3dcaf0d4bae1fd4611120bd4a74e72f568ee7d8424


Requires
--------
python3-stopit (rpmlib, GLIBC filtered):
    python(abi)



Provides
--------
python3-stopit:
    python-stopit
    python3-stopit
    python3.10-stopit
    python3.10dist(stopit)
    python3dist(stopit)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1973680
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Python, Shell-api
Disabled plugins: SugarActivity, Ocaml, C/C++, Haskell, Perl, PHP, Java, R, fonts
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Comment 6 Ben Beasley 2021-06-21 12:42:40 UTC
Thanks, Arthur, for the high-quality preliminary review.

I have noted a couple of remaining small issues below.


Package Review
==============

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


===== Issues =====

- The Summary line is too long; there is an 80 character limit for summary and
  description lines.

    python3-stopit.noarch: E: summary-too-long C Timeout control decorator and context managers, raise any exception in another thread

  I suggest adjusting it to:

    Summary:        Timeout control decorator and context managers

  or

    Summary:        Timeout control, raise any exception in another thread

- Unless you are packaging for EPEL8, %py_provides is no longer required for
  “conventionally-named” Python packages. See
  https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_the_py_provides_macro.
  Note that Fedora 32, which required it, is end-of-life.

  If you are packaging for EPEL8, I would suggest wrapping the macro in a
  conditional to make clear it is not needed on Fedora, something like:

    %if 0%{?epel} == 8
    %py_provides python3-%{pypi_name}
    %endif

  Otherwise, please just remove it.

- Based on the age of the last commit, and on the lack of responses to
  https://github.com/glenfant/stopit/pull/23, I suspect that this package no
  longer has an active upstream. You may still package it; just be aware that
  you are unlikely to have upstream support if issues arise.

===== Notes (no change required) =====

- The description, with the bulleted list, looks great to me.

- Leaving in the shebang removal command is a reasonable packager preference.
  (I would tend not to include it where it is not needed, but you are not wrong
  to do so as a bit of personal boilerplate.) It was also reasonable, of
  course, to point out that it is not required here.

- Please consider trying the pyproject-rpm-macros
  (https://src.fedoraproject.org/rpms/pyproject-rpm-macros) for future new
  packages. While using them is not mandatory, they are very helpful for all
  but a few exceptionally quirky Python packages—especially the generated BR’s
  and automatic file listing. If you’re interested in these, feel free to
  contact me for advice or for help converting a spec file in which you want to
  try them.

===== 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: "Unknown or generated", "Expat License", "*No copyright* Expat
     License". 7 files have unknown license. Detailed output of
     licensecheck in /home/reviewer/1973680-python-stopit/licensecheck.txt

     MIT license is supported by the LICENSE file, README.md,
     https://github.com/glenfant/stopit/pull/23, and especially by the author’s
     statement of intent in
     https://github.com/glenfant/stopit/pull/15#issuecomment-364298245. It
     seems sufficiently clear that the 'GPLv3' in setup.py was an oversight in
     the upstream relicensing, so I agree with the “MIT” license field and with
     patching the 'license' in setup.py.

[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 30720 bytes in 1 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

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]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
[x]: Binary eggs must be removed in %prep

===== 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).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: 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]: 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: python3-stopit-1.1.2-1.fc35.noarch.rpm
          python-stopit-1.1.2-1.fc35.src.rpm
python3-stopit.noarch: E: summary-too-long C Timeout control decorator and context managers, raise any exception in another thread
python3-stopit.noarch: W: spelling-error %description -l en_US callables -> callable, callable s, calculable
python-stopit.src: E: summary-too-long C Timeout control decorator and context managers, raise any exception in another thread
python-stopit.src: W: spelling-error %description -l en_US callables -> callable, callable s, calculable
2 packages and 0 specfiles checked; 2 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Source checksums
----------------
https://github.com/glenfant/stopit/archive/1.1.2/stopit-1.1.2.tar.gz :
  CHECKSUM(SHA256) this package     : df619f61469765aad691bc3dcaf0d4bae1fd4611120bd4a74e72f568ee7d8424
  CHECKSUM(SHA256) upstream package : df619f61469765aad691bc3dcaf0d4bae1fd4611120bd4a74e72f568ee7d8424


Requires
--------
python3-stopit (rpmlib, GLIBC filtered):
    python(abi)



Provides
--------
python3-stopit:
    python-stopit
    python3-stopit
    python3.10-stopit
    python3.10dist(stopit)
    python3dist(stopit)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1973680
Buildroot used: fedora-rawhide-aarch64
Active plugins: Generic, Shell-api, Python
Disabled plugins: PHP, fonts, Perl, R, C/C++, Java, Ocaml, Haskell, SugarActivity
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Comment 7 Ben Beasley 2021-06-21 12:52:27 UTC
Thanks, Rafael. It looks like we found about the same things. (Personally, I think multiple reviews on a package is great!)

> - rpmlint is reporting an error:
>     python3-stopit.noarch: E: summary-too-long C Timeout control decorator and context managers, > raise any exception in another thread

This matches my first noted issue. I suggested some alternatives.

> - The issue with the license is slightly more than a copy-paste error, as it is related to a license change.
> 
> - Upstream was working on the license change in 2017/2018 and did not finish it, yet.

After taking the time to properly review the package and upstream repository, I agree. This is a relic of the old license rather than a copy-paste error. I am, however, convinced by the upstream owner/maintainer’s comment https://github.com/glenfant/stopit/pull/15#issuecomment-364298245 that MIT is indeed the intended license. I think that, considering this, it makes sense to patch the metadata.

> - Upstream code has not been tested with Python 3.7+, and as Python 3.10 is bringing more changes.

This is true. However, I reviewed the package on Fedora 34, using the usual Rawhide mock chroot, which means the test suite at least passed on Python 3.10.

> - Upstream code seems abandoned, is the packager willing to assume/fork the original source code and maintain it in the future?

Also noted. This is a pretty simple package, but maintaining an abandonded packager is always an additional burden. (On the other hand, it’s unfortunately common for abandoned dependencies to linger, even in wide use, in the Python ecosystem, and sometimes the reasonable choice is to keep packaging them.)

Comment 8 Ben Beasley 2021-06-21 12:54:46 UTC
> maintaining an abandonded packager is always an additional burden.

I meant to say an abandoned package, of course. Maintaining an abandoned packager is beyond my expertise!

Comment 9 Aniket Pradhan 2021-06-21 13:44:11 UTC
Thanks a lot Ben, Rafael and Arthur for the reviews. :D

I have fixed the issues in the spec as pointed out by Ben. I would try to use the py_project macros for the next Python project I would package. Thanks for the recommendation.

Comment 10 Ben Beasley 2021-06-21 14:43:16 UTC
I have re-reviewed the package by examining the diff in the spec file:

> --- ../srpm-unpacked/python-stopit.spec 2021-06-20 04:57:23.000000000 -0400
> +++ 1973680-python-stopit/srpm-unpacked/python-stopit.spec      2021-06-21 09:38:53.000000000 -0400
> @@ -16,7 +16,7 @@
>  Name:           python-%{pypi_name}
>  Version:        1.1.2
>  Release:        1%{?dist}
> -Summary:        Timeout control decorator and context managers, raise any exception in another thread
> +Summary:        Timeout control decorator and context managers
>  
>  License:        MIT
>  URL:            https://github.com/glenfant/stopit
> @@ -33,8 +33,6 @@
>  BuildRequires:  python3-devel
>  BuildRequires:  %{py3_dist setuptools}
>  
> -%py_provides python3-%{pypi_name}
> -
>  %description -n python3-%{pypi_name} %_description
>  
>  %prep

All of the issues I found are addressed; the package is therefore approved.

Comment 11 Gwyn Ciesla 2021-06-21 16:13:52 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-stopit

Comment 12 Fedora Update System 2021-06-22 11:14:15 UTC
FEDORA-2021-872c460476 has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2021-872c460476

Comment 13 Fedora Update System 2021-06-22 11:14:16 UTC
FEDORA-2021-1cbbbd0997 has been submitted as an update to Fedora 33. https://bodhi.fedoraproject.org/updates/FEDORA-2021-1cbbbd0997

Comment 14 Fedora Update System 2021-06-23 02:00:50 UTC
FEDORA-2021-872c460476 has been pushed to the Fedora 34 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-872c460476 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-872c460476

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 15 Fedora Update System 2021-06-24 14:46:13 UTC
FEDORA-2021-1cbbbd0997 has been pushed to the Fedora 33 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-1cbbbd0997 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-1cbbbd0997

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 16 Fedora Update System 2021-07-01 01:13:19 UTC
FEDORA-2021-872c460476 has been pushed to the Fedora 34 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 17 Fedora Update System 2021-07-02 01:20:24 UTC
FEDORA-2021-1cbbbd0997 has been pushed to the Fedora 33 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.