Thriftpy / thriftpy2

Pure python approach of Apache Thrift.
MIT License
567 stars 90 forks source link

Investigate build issue involving cython and thriftpy in container build #218

Closed danielballan closed 1 year ago

danielballan commented 1 year ago

Cython recently release 3.0.0. In the container build GHA, I have pinned cython to <3 to work around the error below. Perhaps upstream will fix; we should try un-pinning soon.

  Preparing metadata (setup.py): started
  Preparing metadata (setup.py): finished with status 'error'
  error: subprocess-exited-with-error

  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [28 lines of output]
      Compiling thriftpy2/transport/cybase.pyx because it changed.
      [1/1] Cythonizing thriftpy2/transport/cybase.pyx
      /opt/venv/lib/python3.11/site-packages/Cython/Compiler/Main.py:381: FutureWarning: Cython directive 'language_level' not set, using '3str' for now (Py3). This has changed from earlier releases! File: /tmp/pip-install-l_vvkf17/thriftpy2_dbce685136344ce8a8f1fa2e7857ec70/thriftpy2/transport/cybase.pxd
        tree = Parsing.p_module(s, pxd, full_module_name)

      Error compiling Cython file:
      ------------------------------------------------------------
      ...

          cdef int grow(self, int min_size):
              if min_size <= self.buf_size:
                  return 0

              cdef int multiples = min_size / self.buf_size
                                            ^
      ------------------------------------------------------------

      thriftpy2/transport/cybase.pyx:90:38: Cannot assign type 'double' to 'int'
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/tmp/pip-install-l_vvkf17/thriftpy2_dbce685136344ce8a8f1fa2e7857ec70/setup.py", line 61, in <module>
          cythonize("thriftpy2/transport/cybase.pyx")
        File "/opt/venv/lib/python3.11/site-packages/Cython/Build/Dependencies.py", line 1134, in cythonize
          cythonize_one(*args)
        File "/opt/venv/lib/python3.11/site-packages/Cython/Build/Dependencies.py", line 1301, in cythonize_one
          raise CompileError(None, pyx_file)
      Cython.Compiler.Errors.CompileError: thriftpy2/transport/cybase.pyx
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

× Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.
The command '/bin/sh -c TILED_BUILD_SKIP_UI=1 pip install '.[server]'' returned a non-zero code: 1

Error: Process completed with exit code 1.
danielballan commented 1 year ago

So sorry for the noise---meant to post this against my own repo! :man_facepalming: This may or may not be a thriftpy issue.

uonxhou commented 1 year ago

@danielballan i have the same issue.

aisk commented 1 year ago

This is fixed by #220 .

aisk commented 1 year ago

Thriftpy2 is released by cython precompiled, so users should not got the issue. Do you install thriftpy2 from source?

danielballan commented 1 year ago

Yes, I would have expected to get a wheel but evidently that's not what pip saw when this ran: https://github.com/bluesky/tiled/actions/runs/5598579624/job/15165018609#step:4:746

aisk commented 1 year ago

I mean "from souce" is from the git, not the tar.gz from pypi. But I got what happened in the container.

Seems you have older version of pip / setuptools installed in the container, the older version of pip / setuptools do not use a isolated build environment. And this combination triggered cython to re-compile the .pyx to .c files.

You can upgrade pip / setuptools to get a workaround, before thriftpy2 releases a new version.

danielballan commented 1 year ago

Ah, that makes sense, thank you. Upgrading to latest pip and setuptools in the container is probably wise in any case.