Twisted's external C modules might be worth putting in an external package

[twisted-trac]

	@hawkowl reported
Trac ID	trac#7945
Type	enhancement
Created	2015-06-18 14:40:18Z

This would involve:

• Creating a new package, with the C-exts
• Removing them from Twisted
• Import them into Twisted from the external package

As to why:

Twisted has a handful of (mostly optional) C extension modules, to provide support for the IOCP reactor, some inetd stuff, and sendmsg on Py2. The vast majority of Twisted's users do not require these C modules, and it only makes it harder to install (requiring python-dev + a compiler). If they were bundled into another package and distributed on PyPI, then it would make the majority case simpler, but still allowing easy access to the extension modules.

This has the following direct benefits:

• Twisted users (Tahoe, Crossbar/Autobahn, Buildbot) no longer need to have a C compiler to install Twisted. (It will still be required for things like PyOpenSSL, but that is an optional dependency)
• Twisted can distribute a single wheel and a single source distribution on PyPI. The msi and exe installers and the Windows wheel will only be the C extensions, and therefore much smaller. Since these rarely change, they will not need to be changed on pypi for a long while.
• We can add extra C modules to this package, without having to hard-depend on CFFI in the future.

Searchable metadata

trac-id__7945 7945
type__enhancement enhancement
reporter__hawkowl hawkowl
priority__normal normal
milestone__ 
branch__None None
branch_author__hawkowl hawkowl
status__closed closed
resolution__fixed fixed
component__core core
keywords__None None
time__1434638418108580 1434638418108580
changetime__1603753520219402 1603753520219402
version__None None
owner__graingert graingert

[twisted-trac]

	@hawkowl commented

(In [45090]) Branching to external-cexts-7945.

[twisted-trac]

	@hawkowl commented

This is a branch that does this. It removes the support code from Twisted, and puts it in a new package.

This will not work on the buildbots without some build script modification. The way to test it manually is (from a Twisted checkout):

virtualenv venv
venv/bin/pip install zope.interface
cd cexts
../venv/bin/python setup.py install
cd ..
venv/bin/python bin/trial twisted

To test something that specifically has cexts:

venv/bin/python bin/trial twisted.python.test.test_sendmsg twisted.python.test.test_util.InitGroupsTests

This branch may be backwards incompatible.

[twisted-trac]

	@hawkowl commented

This is related to #3586.

[twisted-trac]

	@adiroiban commented

Many thanks for working on this!

I think that as a first step we should fix the buildbot infrastructure to support such changes.

Regarding the incompatibility part, maybe we can leave deprecated placeholder in the places from were old C module were imported.

In this way, users can just install the new package c extention package and continue to use the old code without updating the imports.

What do you think?

---

Why do we need to import raise in twisted/test/init.py ?

Why not break each extension into its own package?

Are the new c package Twisted specific ?

For example sendmsg look like a generic support for Py2 and could be used in other projects which don't depend on Twisted.

I see that portmap code is somehow reused / duplicated here https://github.com/racker/python-twisted-runner/tree/master/twisted/runner ...

With a separate package python-twisted-runner could just depend on the new package which could be given a generic name like python-rcp-portmap or pyportmap or something like that.

---

I am leaving this in the review queue so that it can receive feedback from the other developers.

Thanks!

[twisted-trac]

	@hawkowl commented

Hi Adi!

The buildbot will need some changes, yes.

Also, this patch re-exports it, so no code needs to be changed -- the backwards incompatibility is that after this, if you "pip install Twisted", you won't get the C modules, even if you have a working C compiler. You would need to install the external C modules.

The importing is to re-export it.

The modules aren't strictly Twisted specific, and there's nothing stopping other things from using them -- but I can nearly guarantee that they're only useful to Twisted.

The python-twisted-runner package is just an export of the existing twisted/runner directory, just split out by Rackspace for some reason. It's like the existing "sub projects" which are going to be removed shortly, so we needn't worry about them.

[twisted-trac]

	@adiroiban commented

Replying to hawkowl:

Hi Adi!

The buildbot will need some changes, yes.

To discuss what kind of changes are needed I have created twisted-infra/braid#108

Also, this patch re-exports it, so no code needs to be changed -- the backwards incompatibility is that after this, if you "pip install Twisted", you won't get the C modules, even if you have a working C compiler. You would need to install the external C modules.

This patch has no newsfile. I think that the newsfile fragment should include a reference to the new package and instruct people to install that package in case they are using C extensions.
.... and maybe we should also include the change as part of the big highlights for the next release.

We can make the new package available as 'twisted[cext]' ... but this can also be done in a separate ticket.

Maybe we can start with defining the new package in install_requires and then deprecate it.

The importing is to re-export it.

OK.

The modules aren't strictly Twisted specific, and there's nothing stopping other things from using them -- but I can nearly guarantee that they're only useful to Twisted.

The python-twisted-runner package is just an export of the existing twisted/runner directory, just split out by Rackspace for some reason. It's like the existing "sub projects" which are going to be removed shortly, so we needn't worry about them.

OK.

Thanks for your comments!

[twisted-trac]

	@hawkowl commented

(In [45243]) Branching to external-cexts-7945-2.

[twisted-trac]

	@glyph set owner to @hawkowl

Heck of merge conflicts, please address them and resubmit :-(.

1. Something something namespace packages. Can't we call this twisted._c or somesuch? Why cexts/? We don't have a src/ or python/ top level directory. If it is going to be a top level package I'd love it to start with an underscore - no need to export a bunch of new public API that people outside Twisted really shouldn't be calling directly.
2. Please just go ahead and import ConditionalExtension under its own name; I was confused by the presence of the conditional callbacks until I looked at the top of the file.
3. Do we really need a separate LICENSE file? Can't we just reference the one at the top level somehow?
4. This really needs to have some release-process documentation shipped along with it. Could you make a draft of an updated ReleaseProcess page or something?

Thanks again for attempting to muck out these C files! Please address the above points and resubmit.

[twisted-trac]

	@glyph commented

Oops, almost forgot: please synchronize the version to Twisted's.

[twisted-trac]

	@hawkowl commented

(In [45438]) Branching to external-cexts-7945-3.

[twisted-trac]

	@hawkowl removed owner

Thanks for the review, Glyph.

• I tried making it a namespace package. But, unfortunately, as you already know, Python. Making Twisted a namespace package means that our bin/etc hacks don't work anymore. So I've gone with a 'private' top level package.
• Done.
• I can't figure out a way to make sdist do it. Maybe when we switch to Git, a symlink will work.
• I've integrated it into the version-changing stuff, so all we'd need to do is an extra step of python setup.py sdist -d /tmp/twisted-release/ in the two instances where we call build-all-tarballs. Either that, or build-all-tarballs also goes and calls setup.py in cexts.
• The version is now synchronised to Twisteds and uses the same machinery so the change-versions script still works.

The other changes that would happen to ReleaseProcess is that we would now have a (py2) .whl as well as a standard source tarball for Twisted. Not sure how we could really do a Py3 wheel (since it's a subset). But that's out of this ticket's scope, this ticket just makes it possible.

Plus, because it's not Twisted as a whole, our .msi and .exe installers probably won't work the same. I would like to replace them with just .whls.

I'm not sure how we're going to go from here. I think we need to modify the buildbots to make this really testable.

[twisted-trac]

	@hawkowl commented

(In [45683]) Branching to external-cexts-7945-4.

[twisted-trac]

	@hawkowl commented

Here's a PR on braid for the changes needed to do this on the buildbots (plus making an OS X 10.10 wheel): https://github.com/twisted-infra/braid/pull/135/files

[twisted-trac]

	@adiroiban set owner to @hawkowl

Hi,

hawkowl are you going to continue working on twisted-infra/braid#135 and put it for review ?

I would like to have the at least one Linux / OSX and Windows builder to check the new code.

For the LICENCE file, maybe we can create a custom sdist step which will auto-copy the base LICENSE.

I think that distributing .whl files should be fine and we no longer need .msi/.exe

---

Just a minor comment.
I am not quiet happy about the way changeAllProjectVersions got a flag argument.

Why not create a separate method and call it changeCExtVersions ?

---

in cexts/_twistedextensions/tests/test_dist.py

I see this code

+from twistedextensions._dist import ConditionalExtension, get_setup_args
+from twistedextensions._dist import _checkCPython

is it ok... why not use _twistedextensions

---

pip install _twistedextensions

look strange ... for my taste.

Why do we need the private namespace?

Why not call it txcext ?

---

Many thanks for your work.

Please check my comments and resubmit.

Also please add a few notes about how someone is supposed to test the changes.

Thanks!

[twisted-trac]

	@hawkowl removed owner

I think this is a good time for a review.

See: https://pypi.python.org/pypi/Twisted_platform_support for the manylinux wheels and https://github.com/twisted/twisted-platform-support for the repo

The changes on this branch directly test that package. It means that Windows and Linux don't need a compiler for basically all the Pythons.

[twisted-trac]

	@hawkowl commented

(pr at #617)

[twisted-trac]

	@glyph commented

Definitely planning to do a fuller review later, but: I am inclined to agree with Adi's earlier feedback that this should be _twisted_platform_support. Nobody outside of Twisted should be depending on this stuff directly, and the name should reflect that.

[twisted-trac]

	@glyph commented

Also, please grant PyPI access to the project to all current PyPI owners of Twisted, as well :).

[twisted-trac]

	@hawkowl commented

This will need to go through the compat exception process, it removes the (disused) portmap.c with no replacement, as it is a maintenance burden. Email forthcoming.

[twisted-trac]

	@glyph commented

I'm not necessarily opposed to that - portmap is nonsense, non-portable, and obviously nobody uses it - but it seems that compat-excepting this is a bit beside the point. We could simply include it in the new platform support package.

[twisted-trac]

	@hawkowl commented

Yeah, agreed. I did, and deprecated it in Twisted.

[twisted-trac]

	@hawkowl commented

Wow, isn't this a ticket? :)

So, with some discussions with glyph and co, I have the following MULTI-POINT PLAN:

1. Instead of bundling it with the rest, moving IOCPReactor into its own package, with its own external API, that does not operate in lockstep. IOCPReactor would have nightly builds against Twisted current, and Twisted trunk. Twisted's Windows builders would test against IOCPReactor current, and IOCPReactor trunk.

2. _twisted_platform_support becomes _twisted_c_extensions and sits next to Twisted in src/. It has the same version as Twisted. Twisted, with the setuptools requirements, will depend on the same version as it.

3. Both these projects will build and distribute binaries via PyPI, done automatically by Travis and Appveyor.

Sound good?

[twisted-trac]

	@glyph commented

Replying to hawkowl:

Wow, isn't this a ticket? :)

So, with some discussions with glyph and co, I have the following MULTI-POINT PLAN:

I think it's important to note the motivations behind each point.

1. Instead of bundling it with the rest, moving IOCPReactor into its own package, with its own external API, that does not operate in lockstep. IOCPReactor would have nightly builds against Twisted current, and Twisted trunk. Twisted's Windows builders would test against IOCPReactor current, and IOCPReactor trunk.

The goal here is to have all the existing Windows test infra test some of the most Windows-sensitive components of Twisted, and to make it sensible to make updates to one project without having to span two codebases. By moving IOCPReactor out of Twisted rather than just the support modules, it will be easier to write updates to IOCPReactor itself as they may span between the support modules and the Python implementation code.

Also worth noting: iocpsupport et. al. - basically everything except the reactor itself - will be deprecated in Twisted and private in its new location. The only public API for the new IOCPReactor package will be the public reactor APIs (and perhaps some semi-public stuff that leaks through like addActiveHandle).

Technically this means it will be possible to violate Twisted's compatibility policy if we make an update to iocpreactor and someone were to pin the Twisted version but not pin the IOCPReactor version, but I think that's certainly against the spirit of said policy - if you want compat, pin versions of everything :)

2. _twisted_platform_support becomes _twisted_c_extensions and sits next to Twisted in src/. It has the same version as Twisted. Twisted, with the setuptools requirements, will depend on the same version as it.

This is so that we don't need to add public APIs for our test helpers or find ourselves having to do cross-project releases just to fix a bug, and so that installing from git+https://github.com/twisted/twisted still works as expected. However, this is really a "just for now" solution since it seems like we could give all our other C code a public API; once we've deleted portmap at least.

3. Both these projects will build and distribute binaries via PyPI, done automatically by Travis and Appveyor.

More release automation is just generally a good thing :).

Sound good?

Sounds good to me.

[twisted-trac]

	@glyph set owner to @hawkowl

OK, so, since the next action here is on the part of the author, and it's because of some stuff I said, I'm going to mark this as "reviewed" (by me, I did it)

[twisted-trac]

	@hawkowl commented

Maybe this is as good of a time as any to get a review. Note: this does not include any IOCPReactor changes.

PR: #812

[twisted-trac]

	@hawkowl removed owner

[twisted-trac]

	@glyph commented

Update description to remove things that hopefully don't exist any more.

[twisted-trac]

	@glyph set owner to @hawkowl

Looks surprisingly simpler than I expected, but I do think we need one more pass:

1. Could you move the regeneration of _raiser.c into a separate ticket, that we can land first? I feel like reviewing that in isolation would be easy but the diff generates a lot of noise here. I'm curious as to why this was necessary - did some new setuptools stuff not work with the old version on Windows somehow?
2. What is the deal with ignoring MANIFEST? That worries me, but just because I just don't understand it.
3. Please update the documentation as described in github comments so that any documentation that covers functionality which won't work any more with pip install twisted it's clear how to get it working again. (There appears to be mercifully little of this).
4. Did you ever do that draft update of ReleaseProcess somewhere that was requested in the previous review?

Please address these points and resubmit; hopefully once the _raiser.c stuff is dealt with separately it will be quick to review it all. Thanks!

[twisted-trac]

	@hawkowl removed owner

I'm happy to put this up for re-review.

WRT review points:

Could you move the regeneration of _raiser.c into a separate ticket, that we can land first? I feel like reviewing that in isolation would be easy but the diff generates a lot of noise here. I'm curious as to why this was necessary - did some new setuptools stuff not work with the old version on Windows somehow?

This is done. It had to be regenerated because the name is now _raiser instead of raiser. It's dumb.

What is the deal with ignoring MANIFEST? That worries me, but just because I just don't understand it.

MANIFEST is what is made if you don't have a MANIFEST.in. We should never have one outside of the dist dir (in the wheels, for example), and it shouldn't be checked in, a broken setup.py test added one so I added it as a future safeguard.

Please update the documentation as described in github comments so that any documentation that covers functionality which won't work any more with pip install twisted it's clear how to get it working again. (There appears to be mercifully little of this).

Done.

Did you ever do that draft update of ReleaseProcess somewhere that was requested in the previous review? 

Done.

[twisted-trac]

	@markrwilliams commented

#812 (review)

One typo and some suggestions around the dev extra.

Also, we'll need to update https://twistedmatrix.com/trac/wiki/TwistedDevelopment#Creatingyourworkenvironment to include installing twistedcextensions.

Thanks so much for this multi-year effort - I'm excited to get this merged!

[twisted-trac]

	@graingert set owner to @graingert

[twisted-trac]

	@graingert commented

#1446

[twisted-trac]

	@graingert set owner to adiroiban1

[twisted-trac]

	@wsanchez set owner to @graingert

[twisted-trac]

	@wsanchez set status to closed