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

Require cython when installing from a PyPI sdist? #1076

Closed
jamadden opened this Issue Jan 22, 2018 · 6 comments

Comments

Projects
None yet
1 participant
@jamadden
Member

jamadden commented Jan 22, 2018

Currently, when we upload an sdist to PyPI, it contains a single C file generated by Cython. This C file has to be valid for all platforms (operating system and version of Python) that could install the sdist.

We accomplish this through the use of the cythonpp script which merges the results of running cython multiple times with various C preprocessor directives defined in different ways. It takes ~45 seconds to run on my workstation, and produces a resulting C file that's around 50% larger than it would be for a single platform (potentially leading to longer compile times).

For gevent 1.3, we're adding support of Python 3.7. Currently that means that we need to use a pre-release git clone of cython to produce this merged C file, so if there are any bugs, they will affect everyone who installs from the sdist, no matter if they're running on a different version of Python.

It also means that we can't use the cython directives IF and friends, since those are lost in the generated C file.

A run of cython alone without the use of cythonpp takes around 2 seconds (there will still have to be some use of cythonpp because there are a few preprocessor directives we still need to account for, but not near as many if we can use IF---exact numbers to follow). On Unix, we rely on a Makefile to make this bearable during development---although, if you're actually developing the Cython C extension, it doesn't help much. On Windows we don't have that luxury at all and simply run all the required commands each time. This leads to the logic being duplicated two places (Makefile and make.cmd), which is not nice. If cythonpp is bearable all the time, we could collapse that duplication.

Most installs of gevent are from a binary nowadays, I believe, so this wouldn't affect most people.

Other than adding an install-time dependency, which gevent currently does not have, this does open up the possibility of compiling gevent with untested versions of Cython; if that becomes an issue we could probably use a sufficiently tight version constraint.

@jamadden

This comment has been minimized.

Member

jamadden commented Jan 22, 2018

Also, it turns out cythonpp has a terrible bug where it can sometimes produce merged files that cannot compile on some versions of Python (depending on what's implemented as a macro and what's a function). I just wasted a lot of time finding that out and trying to fix it.

@jamadden

This comment has been minimized.

Member

jamadden commented Jan 22, 2018

cythonpp has a different bug, or a different manifestation of the same bug, where it apparently doesn't simimplify tags correctly. Witness this WIN32 build that failed due to a missing init symbol, while the corresponding Travis build all passed. A few score lines above where the symbol was defined, at the end of a completely unrelated function was this code:

static int __Pyx_InitCachedBuiltins(void) {
...
#if (!defined(LIBEV_EMBED) && !defined(_WIN32)) || (defined(LIBEV_EMBED) && !defined(_WIN32))
  return 0;
  __pyx_L1_error:;
  return -1;
}

and then a hundred or so lines later, after Cython has defined (and is in the process of implementing) the missing symbols, an unrelated function has:

#endif /* (!defined(LIBEV_EMBED) && !defined(_WIN32)) || (defined(LIBEV_EMBED) && !defined(_WIN32)) */
#if (!defined(LIBEV_EMBED) && !defined(_WIN32))
  if (PyDict_SetItem(__pyx_d, __pyx_n_s_LIBEV_EMBED, Py_False) < 0) __PYX_ERR(0, 1159, __pyx_L1_error)
#endif /* (!defined(LIBEV_EMBED) && !defined(_WIN32)) */

Smack in the middle of that block of code that was completely disabled for WIN32 was the missing symbol.

Fortunately, I've just about got it down to the place that I don't think we need cythonpp anymore at all, so that's good.

@jamadden

This comment has been minimized.

Member

jamadden commented Jan 22, 2018

FWIW, as of ea0bd82 we're down to generating 22K LOC (from 35K LOC) and running in 4.5s (down from 45s). That's with one CPP define still being used in the script.

@jamadden

This comment has been minimized.

Member

jamadden commented Jan 23, 2018

The use of cythonpp has been eliminated with #1077.

Another good reason for requiring cython at sdist installation time: that way the git tag can match the contents of the sdist. Currently, the sdist has the generated C file that's simply not in the repository at all. The generated source features strings (like datestamps and file paths) that mean that it can't be reproduced exactly ever again.

Now, I think the embedding of a datestamp and a random temporary file path are gone with dropping cythonpp, so the exact file can probably be reproduced now. Maybe. But I would still prefer to avoid distributing something not tagged.

@jamadden

This comment has been minimized.

Member

jamadden commented Jan 23, 2018

One last hurdle to simply calling cythonize from setup.py is that the Makefile currently modifies the generated source by taking on an extra include at the end.

@jamadden

This comment has been minimized.

Member

jamadden commented Jan 23, 2018

But I would still prefer to avoid distributing something not tagged.

So cython still suggests distributing generated c files as a best practice.

jamadden added a commit that referenced this issue Jan 23, 2018

Run cythonize from setup.py
This eliminates our sdist-time dependency on Makefile or make.cmd on
Windows. Fewer moving pieces are better.

Fixes #1076

jamadden added a commit that referenced this issue Jan 23, 2018

Run cythonize from setup.py
This eliminates our sdist-time dependency on Makefile or make.cmd on
Windows. Fewer moving pieces are better.

Fixes #1076
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment