Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cython dependency at the installation #386

Closed
biernackip opened this issue Sep 11, 2014 · 8 comments
Closed

Cython dependency at the installation #386

biernackip opened this issue Sep 11, 2014 · 8 comments
Assignees
Labels
Milestone

Comments

@biernackip
Copy link

With newest Cython (0.21, at the moment of writing) PyTables do not want to install. Downgrading to version 0.15 helped. Cython 0.21 is simply not recognized.

@nathanfranklin
Copy link

I am experiencing the same problem with Cython 0.21. Cython 0.20 was working. The following change works for Cython .21 and .20

diff --git a/setup.py b/setup.py
index 0f2f3f7..f289390 100755
--- a/setup.py
+++ b/setup.py
@@ -123,7 +123,7 @@ if not has_setuptools:
 # Check if Cython is installed or not (requisite)
 try:
     from Cython.Distutils import build_ext
-    from Cython.Compiler.Main import Version
+    from Cython.Compiler import Version
     cmdclass['build_ext'] = build_ext
 except ImportError:
     exit_with_error(

@waylonflinn
Copy link

I ran into the same problem last night. It looks like this was fixed in the develop branch back in June. We just need a merge and new release.

Relevant Commit: e1dd583

@avalentino avalentino added this to the 3.2 milestone Sep 11, 2014
@avalentino avalentino self-assigned this Sep 11, 2014
@avalentino avalentino added defect and removed invalid labels Sep 11, 2014
@avalentino
Copy link
Member

Hi guys, thanks for reporting.

I ran into the same problem last night. It looks like this was fixed in the develop branch back in June. We just need a merge and new release.

Relevant Commit: e1dd583

Yes it should be already fixed in the develop branch.

Anyway compayibility with old PyTables versions has been restored in Cython: cython/cython@43342ab

@avalentino
Copy link
Member

See also #387.

@scoder
Copy link

scoder commented Sep 17, 2014

As a general remark, it's usually a good idea to not use Cython "just because it's there", as that makes it less predictable what users get on their side. They may have an old or buggy Cython version and thus get suboptimal performance or even incorrect behaviour in some cases. I'm not saying that using an installed Cython for the build is wrong, I just do not consider it best practice.

Instead, ship the generated (and tested) .c files with source distributions and require users to explicitly request their regeneration if they want to and know what they're doing (I commonly use a "--with-cython" setup.py switch for that). Only run Cython automatically if the .c files are really missing during the build (or if they are outdated in development checkouts).

@avalentino
Copy link
Member

Hi @scoder, thanks for the advice.

Currently we adopt a somewhat mixed policy:

  • we include the generated .c files in the "official" tarball of all versions (the one that users can
    download from sf.net or PyPi). Official tarballs are the only ones to be used in production.
  • we do not include any automatically generated file (.c, .html, etc)

The underlying assumption is that people that grab code from a git repo is reasonably skilled people and that they is also able to correctly set up a development environment for pytables.

Do you think it makes sense?
Of course there is always room to improve our policy :)

@scoder
Copy link

scoder commented Sep 18, 2014

we do not include any automatically generated file

I read "Include" as "commit in VCS" here, right? I totally agree that generated files should not be under version control. However, your setup.py currently requires Cython to be installed unconditionally, although it's not needed for "normal" users (who use production-ready releases, as you say). I see now that it's only used if the .c files are missing or the sources were changed, though, so that's ok (and matches what I was suggesting). Under normal conditions then, the released .c files will not be overwritten.

Anyway, it works the way it is, so sorry for the noise.

@avalentino
Copy link
Member

we do not include any automatically generated file

I read "Include" as "commit in VCS" here, right? I totally agree that generated files should not be under

yes, correct

version control. However, your setup.py currently requires Cython to be installed unconditionally,

yes, you are right, Cython should be only required if .c files are missing or outdated.
If i remember well we we forced it to be unconditionally installed because several users reported problems that was due to out of sync .c and .pyx/.py files.
It is more a workaround than a real solution and the setup.py script actually needs a radical rewrite (see also #122), so any hint in this field is very welcome.

thanks for your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants