Bug 233561 - converters/py-rencode: Fails to configure if cython is installed
Summary: converters/py-rencode: Fails to configure if cython is installed
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Kubilay Kocak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-27 12:28 UTC by John Hein
Modified: 2018-11-30 02:34 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (python)


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description John Hein 2018-11-27 12:28:59 UTC
If one tries to build py-rencode with cython installed, it fails during the configure phase.  Excerpt below shown when py27-cython-0.28.2 was installed.  py27-cython-0.29 is similar (line number differences in traceback).

=======================
===>  Configuring for py27-rencode-1.0.6
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "setup.py", line 53, in <module>
    ext_modules = cythonize(ext_modules)
  File "/usr/local/lib/python2.7/site-packages/Cython/Build/Dependencies.py", line 897, in cythonize
    aliases=aliases)
  File "/usr/local/lib/python2.7/site-packages/Cython/Build/Dependencies.py", line 777, in create_extension_list
    for file in nonempty(sorted(extended_iglob(filepattern)), "'%s' doesn't match any files" % filepattern):
  File "/usr/local/lib/python2.7/site-packages/Cython/Build/Dependencies.py", line 102, in nonempty
    raise ValueError(error_msg)
ValueError: 'rencode/rencode.pyx' doesn't match any files
*** Error code 1

Stop.
make: stopped in /usr/ports/converters/py-rencode
=======================
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2018-11-28 04:44:44 UTC
Investigation summary:

1) ports r482774 removed USE_PYTHON=cython (inadvertently, due to (2), or because the package successfully built without cython)
2) upstream setup.py does not declare cython as a dependency
3) upstream dev-requirements.txt does declare it as a dependency (noop for setuptools/pip based installs as they use setup.py:*_requires)
4) upstream setup.py conditionally (when importable/found) uses Cython (.pyx file) for the build, falling back to a setuptools build_ext (.c file) build
5) The 1.0.6 PyPI sdist (only) ships with a .c file
6) Upstream commit 5c928f14567fabc9efb8bbb8ac5e0eef03c61541 via issue #25 [1] adds a pyx file (not yet released).
7) setup.py declares 'sdist requires cython module to generate c' file, appearing to indicate it (cython) is a development dependency, prior to, and for producing, the relevant source distribution (which we use).

In my opinion, a cython build (and the dependency on cython) it not necessary (particularly given (5) and (7)), even after #25 is released and the conditional code to build with cython should not be included in setup.py, at least not as a default case, or without an explicit request by the user to use cython, as an optional build method, for sdist consumers (most downstream packagers, and setuptools/pip users)

[1] https://github.com/aresch/rencode/commit/5c928f14567fabc9efb8bbb8ac5e0eef03c61541
[2] https://github.com/aresch/rencode/pull/25
Comment 2 commit-hook freebsd_committer freebsd_triage 2018-11-28 05:52:05 UTC
A commit references this bug:

Author: koobs
Date: Wed Nov 28 05:51:50 UTC 2018
New revision: 486079
URL: https://svnweb.freebsd.org/changeset/ports/486079

Log:
  converters/py-rencode: Fails to configure if cython is installed

  ports r482774 removed USE_PYTHON=cython, likely due to it (cython) not
  being explicitly declared as a dependency in setup.py:*_requires.

  However, setup.py conditionally builds with cython if it is installed, but
  the 1.0.6 source distribution (sdist) does not contain a .pyx file to build
  with.

  This leads to a configure/build error when cython is installed:

    ValueError: 'rencode/rencode.pyx' doesn't match any files

  Upstream commit 5c928f14567fabc9efb8bbb8ac5e0eef03c61541 [1] via issue
  #25 [2] adds the required .pyx file to the sdist, which technically
  addresses the "build with cython from the sdist" issue, but does not
  fundamentally resolve the higher-level question: why build with cython when
  a C source pre-processed by cython has already been produced for, and is
  contained in, the source distribution.

  A cython build (and the dependency on cython) does not appear to be
  necessary, nor intended [3][4], nor recommended [5], even after #25 is
  released, for sdist consumers.

  The conditional code to build with cython should not be included in
  setup.py, at least not as a default case, or without an explicit request
  by the user to use cython, as an optional build method, for sdist consumers
  (most downstream packagers and setuptools/pip users [6]).

  This change removes the conditional check for Cython, leaving the standard
  setuptools build_ext (with the packaged .c) file, as the build method.

  While I'm here:

   - Canonicalise COMMENT (match seutp.py:description)
   - Add test target with post-patch target to make the tests dir a module
     usable by a setup.py test (via test_suite directive) target.
   - Honour CFLAGS: Remove forced (appended) -O3 arg from setup.py

  [1] https://github.com/aresch/rencode/commit/5c928f14567fabc9efb8bbb8ac5e0eef03c61541
  [2] https://github.com/aresch/rencode/pull/25
  [3] setup.py: "Error: sdist requires cython module to generate `.c` file."
  [4] dev-requirements.txt:Cython
  [5] https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#distributing-cython-modules
  [6] https://github.com/aresch/rencode/issues/11

  PR:		233561
  Reported by:	John Hein <z7dr6ut7gs snkmail com>
  Approved by:	koobs (python)

Changes:
  head/converters/py-rencode/Makefile
  head/converters/py-rencode/files/
  head/converters/py-rencode/files/patch-setup.py
Comment 3 Kubilay Kocak freebsd_committer freebsd_triage 2018-11-28 06:02:38 UTC
Committed, thank you for the report John
Comment 4 John Hein 2018-11-29 18:52:11 UTC
Thanks for the fix, koobs.

I got as far as hard-coding cythonize=False in setup.py locally.  But your commit (and the excellent explanation herein) is far more satisfying.

Was this raised upstream?  I didn't get around to it yet (because I had not done the analysis that you did and did not understand the history).  As you observed there are some inconsistencies (in both docs and code) that could be addressed upstream.
Comment 5 Kubilay Kocak freebsd_committer freebsd_triage 2018-11-30 02:34:58 UTC
(In reply to John Hein from comment #4)

You're welcome John

I didn't raise the issue explicitly, *but*, when commit log messages contain git(hub) issue/pr/commit references, as were included here, references are added as comments to those respective GitHub objects, which I also believe authors and subscribers are notified of. I intentionally add as many GitHub references to my commits for this reason, so upstreams are implicitly notified of activity at (FreeBSD) downstream, thereby raising awareness. The detailed commit log message is also designed also as much for upstream, as it is for us :)

I'll probably send a PR upstream fixing this and a few packaging issues at some stage, if they haven't already been fixed or improved in the meantime due to the extra notifications :)