Skip to content

Autogenerate and include C files in sdist#459

Closed
jvkersch wants to merge 3 commits into
masterfrom
maint/include-c-files
Closed

Autogenerate and include C files in sdist#459
jvkersch wants to merge 3 commits into
masterfrom
maint/include-c-files

Conversation

@jvkersch
Copy link
Copy Markdown
Contributor

@jvkersch jvkersch commented Jul 5, 2019

This is a first attempt at addressing #333

This PR changes the build procedure so that:

  1. Cythonized C files are not checked into the repo any more. Creating an sdist causes Cython to run, and the generated files are added to the sdist.

  2. Installing a development version requires Cython to be installed; rebuilding the extension modules will cause Cython to be run.

  3. Installing a source distribution does not require Cython to be installed. If Cython is available, it will be used to compile the pyx files, if it is not the C files in the sdist will be used.

@corranwebster @rkern I'm interested in hearing your take since you opened the original issue. One thing in particular that I don't get is why we don't make Cython a hard dependency and always run it. That would make the current approach a bit simpler, but maybe I'm overlooking something.

@mdickinson
Copy link
Copy Markdown
Member

How bad would it be if installing from an sdist never used Cython? I kinda dislike the idea that the result of a pip install depends on whether you happen to have Cython installed or not, and what the Cython version happens to be.

@jvkersch
Copy link
Copy Markdown
Contributor Author

jvkersch commented Jul 8, 2019

How bad would it be if installing from an sdist never used Cython?

@mdickinson I agree we should eliminate any source of arbitrariness in the build process. The only thing that I'm not sure about is how to figure out whether a user is installing from an sdist. Is there a special way to detect that?

@jvkersch
Copy link
Copy Markdown
Contributor Author

jvkersch commented Jul 8, 2019

The other option is of course to make Cython a hard dependency and to let Cython generate the C files if and when it needs to. Are there any reasons why we can't do that? The discussion in #333 wasn't entirely clear to me.

@rkern
Copy link
Copy Markdown
Member

rkern commented Jul 8, 2019

These days, I'm happy to just require Cython as a build dependency for ETS packages.

@corranwebster
Copy link
Copy Markdown
Contributor

FWIW I think we require SWIG to build enable from source, so having Cython as a dependency is probably not the worst thing in the world. It's probably not a huge ask for them to have a C compiler and Cython.

I think the reasoning at the time of opening the issue was just an instinct to want the best of both worlds, but looking at the complexity that it adds to the setup.py I'm not sure that its worth the cost.

@jvkersch
Copy link
Copy Markdown
Contributor Author

Superseded by #502

@jvkersch jvkersch closed this Dec 31, 2019
@jvkersch jvkersch deleted the maint/include-c-files branch December 31, 2019 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants