Opened 10 years ago
Closed 8 years ago
#4696 enhancement closed fixed (fixed)
client endpoint: process
Reported by: | Glyph | Owned by: | therve |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | endpoint |
Cc: | Branch: |
branches/child-processes-endpoint-4696-6
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | exarkun, ashfall, tomprince |
Description (last modified by )
It should be possible to make a client endpoint like this:
clientFromString('process:imapd')
which would run the imap daemon in a subprocess and talk to it directly, rather than connecting to it over TCP.
As a sub-task (possibly big enough for a separate ticket) we should have one, official, blessed transformer that adapts from a process transport to a regular stream transport.
Change History (49)
comment:1 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 10 years ago by
Keywords: | endpoint added |
---|
comment:3 Changed 10 years ago by
Owner: | Glyph deleted |
---|
comment:4 Changed 9 years ago by
Owner: | set to ashfall |
---|
comment:5 Changed 9 years ago by
Author: | → ashfall |
---|---|
Branch: | → branches/child-processes-endpoint-4696 |
comment:6 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | ashfall deleted |
comment:7 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to ashfall |
Thanks!
- There are some conflicts. Please merge forward and resolve them.
- There are some missing features:
IProtocol.makeConnection
needs to be called when the connection is established. This is going to be a job for_WrapIProtocol
.IProtocol.dataReceived
needs to be called sometimes! :) Another job for_WrapIProtocol
.- The protocol needs to be given a transport that implements
ITransport
, with its various methods -write
,writeSequence
,loseConnection
,getHost
,getPeer
, etc (check the interface for the complete list).
- It's probably a good idea to just refer to
IReactorProcess.spawnProcess
for the parameter API documentation, rather than reproducing all of it inProcessEndpoint.__init__
's docstring (fewer places to update if we make improvements or other changes). - This assertion in the tests isn't useful:
self.assertEqual(self.ep._wrappedPP.processEnded(connectionDone), self.ep._wrappedPP.protocol.connectionLost(connectionDone))
. What this proves is that the return value of_wrappedPP.processEnded
is equal to the return value ofprotocol.connectionLost
. What you really want to say is that ifprocessEnded
is called on the wrapper,connectionLost
gets called on the application protocol. The way to go about this is the same as how you didtest_spawnProcess
- define the method that's going to get called, implement it to save its arguments somewhere, and then verify those arguments after you expect it to have been called (this is what you'll want to do withmakeConnection
anddataReceived
, too -makeConnection
is half done for you, since the baseProtocol
implements that to save its argument as thetransport
attribute of the protocol instance). self._protocol
andself._wrappedPP
should go away. They don't make sense, given thatconnect
can be called multiple times._WrapIProtocol.processEnded
needs to fiddle with thereason
argument a little bit. Process protocols get handed a reason likeProcessDone
orProcessTerminated
.ProcessDone
with a status of0
means "clean exit" (by POSIX convention, anyway). "clean exit" is sort of liketwisted.internet.error.ConnectionDone
.ProcessDone
with any other status, orProcessTerminated
, carries some extra information about potential errors. I'd like to give you definitive instructions on what to do with them, but I'm not quite sure right now. One option would be to turn them intoConnectionLost
, which is howIProtocol
gets told the connection did not end "cleanly" (ie, data may have been lost). On the other hand,ConnectionLost
doesn't convey as much information as aProcessTerminated
instance would. So another option would be to just pass the original exception through in these error cases. That might be surprising though, sinceIProtocol
will never be given that kind of exception in any existing cases. That might be fine though, since this is a new API. It might be a good idea to solicit further feedback on IRC about opinions on this.- Some of the
ProcessEndpoint
parameters might not make sense. For example, a protocol only has one input (protocol.dataReceived
) and one output (transport.write
).childFDs
can be used to set up multiple inputs and outputs, though. It's not clear how data would move around in that case. Perhaps we can discuss which of these can work at our meeting this week.
Child processes are complicated and subtle, so I'm not surprised at all that this branch needs some more work. It's a great start though. Thanks!
comment:8 Changed 9 years ago by
Branch: | branches/child-processes-endpoint-4696 → branches/child-processes-endpoint-4696-2 |
---|
(In [34681]) Branching to 'child-processes-endpoint-4696-2'
comment:9 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | ashfall deleted |
Addressed the review comment.
comment:10 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to ashfall |
Thanks! Here's some more feedback:
- twisted/internet/test/test_endpoints.py
_TestProtocol
and_TestFactory
need documentation and probably better names. Also note that_TestFactory
could almost as easily be replaced by just aFactory
instance with itsprotocol
attribute set.- On
_TestProtocol
,dataReceived
andconnectionLost
aren't allowed to return anything exceptNone
, so_TestProtocol
doesn't behave like a nice protocol. It looks like these methods return their argument so that the value can be checked later on. The way to do this is to save the argument as an attribute on the instance and then check the attribute later on. _FakeProcessTransport
also needs documentation. Also, a convention Twisted is working on adopting is using "Memory" in the name for this kind of fake, instead of "fake"._FakeProcessTransport.writeToChild
ignores thechildFD
argument. This means the tests don't demonstrate data is ever being written to the child's stdin. It could be written to any other file descriptor without the tests realizing it.True
isn't a useful return value for any of the other methods on_FakeProcessTransport
._FakeProcessReactor
needs documentation._FakeProcessReactor.spawnProcess
should not be setting any attributes on the process protocol passed to it. It *may* want to callmakeConnection
on it, but I'm not sure it even needs to do that. Setting of this attribute doesn't seem to be relied on anywhere, so it probably just makes sense to delete it. Here's where you might want to return that_FakeProcessTransport
instance though.ProcessEndpointsTestCase
is a little bit confusing/confused. Is it a container for tests for the endpoint, for_WrapIProtocol
, for the fake transport implementation? It contains tests for all of these things. I think the test method organization needs to be cleaned up a bit. Have oneTestCase
subclass for_WrapIProtocol
behavior. Have another forendpoints._ProcessEndpointTransport
. Have a third for the endpoint itself.test_writeToChild
, in particular, is a unit test only for testing infrastructure. It definitely belongs separated from tests for the public endpoint APIs and functionality.
ProcessEndpointsTestCase.setUp
creates aProcessEndpoint
with a nonsenseexecutable
- a_FakeProcessTransport
instance. The executable is supposed to be the filesystem path to the program to run.test_wrappedProtocol
still doesn't assert something useful. We might have talked about this on IRC, but I guess I didn't write about it in my last review: it's not very interesting that there is a_wrappedPP
attribute or that it is an instance ofProcessProtocol
. It's slightly more interesting to assert that it's an instance of_WrapIProtocol
, so you could update the test to do that.test_spawnProcess
is a good start. However, neither it nor any other tests actually pass any interesting values to the endpoint. Nearly everything ends up with the default values. The test suite would benefit from an additional test that exercises support for passing each of these additional parameters.ProcessEndpointTransportTests
's docstring says it is for testing_WrapIProtocol
, but it seems to be for testingendpoints._ProcessEndpointTransport
.test_ProcessEndpointTransportInstance
is misnamed. Also, I don't think either of its assertions are very useful. The first assertion, at least, is entirely redundant: it must be an instance of that type, because setUp made it so.test_loseConnection
,test_getHost
,test_getPeer
, should all be a bit different.True
isn't a meaningful return value for any of those methods.WrappedIProtocolTests.setUp
also constructs aendpoints.ProcessEndpoint
with a nonsense executable.test_makeConnection
should not callself.ep._wrappedPP.makeConnection
. Instead, either the fake reactor should callmakeConnection
on the process protocol passed to it, or the fake reactor should remember the protocol passed to it and the test method should that memory (ie an attribute) to find the object to callmakeConnection
on. Calling it onself.ep._wrappedPP
is unrealistic, since in realitymakeConnection
will only be called on the object passed tospawnProcess
, not on a private attribute of the endpoint. It's true that they're the same object in this case, but there's no unit tests demonstrating this fact.- The next line after the
makeConnection
call,self.assertEqual(self.ep._wrappedPP.protocol.transport, self.ep._wrappedPP.transport)
, has too many .s in it. Use a more local reference to the application protocol (ie, the object referred to byself.ep._wrappedPP.protocol
): the test created it (by way of_TestFactory
), the test should be able to get it more easily. test_logStderr
callslog.addObserver
. UseTestCase.addCleanup
to undo this. It's very important to undoaddObserver
calls, since they affect global mutable state and typically come with a significant cost. :)- Same goes for the other tests that call
addObserver
. test_stdout
runs into the same problem with values being returned from the protocol'sdataReceived
. It needs to check instance state, not try to pass the data back via return values.test_processDone
andtest_processEnded
might want to useFailure.check
. And it needs to not use the return value trick.
I think I'll leave the comments at this for now, and let you find any necessary improvements in the implementation as a consequence of improving the tests (since the goal with test driven developments is for the tests to guide you to a good implementation :).
Thanks!
comment:11 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | ashfall deleted |
Fixed the issues raised in the review (and some extra stuff that needed fixing).
comment:12 follow-up: 13 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to ashfall |
twisted/internet/process.py
- There's some new trailing whitespace in this file.
- The docs for
_ProcessEndpointTransport.__init__
repeat the same information twice. The@param
should explain more about the object (or be omitted). For example, I'd write something like "An active process transport which will be used by write methods on this object to write data to a child process." twisted.python.components.proxyForInterface
will probably make_ProcessEndpointTransport
shorter by removing the proxying boilerplate forloseConnection
,getHost
, andgetPeer
.- The
errFlag
accepted by_WrapIProtocol
and the endpoint class isn't expandable.True
andFalse
have meanings now, and there are no other possible values for a boolean. How abouttwisted.python.constants.Names
to define some constants for this? - The
transport
attribute of_WrapIProtocol
is undocumented and also unused, except for the line immediately following its definition. It can probably be removed entirely. _WrapIProtocol
'schildDataReceived
andprocessEnded
methods are undocumented.Failure.check
is a better choice thantype(someFailure.value) == SomeException
.ProcessEndpoint.connect
's docstring is a little light on details. How about "Launch a child process and connect it to a protocol created by C{factory}."? The current docstring doesn't say much that a reader couldn't guess. The docstring should try to convey extra information that the reader wouldn't know already.ProcessProtocol.connect
returns aDeferred
that fires with the wrong object.IStreamClientEndpoint.connect
is meant to return aDeferred
that fires with the protocol that has been connected.- For the stderr logging case,
log.msg(data)
is basically correct. However, two things:- We're trying to move towards "structured" logging. This means that any time you log something, it should probably look more like this:
log.msg(format="foo %(bar)s baz", bar=quux)
. This makesquux
available as part of the event dictionary, should an observer be interested in it specifically. - The extra formatting also makes it easier for someone reading the log to understand what the log message means. In this case, I suggest identifying the child process executable and the application protocol associated with it, in addition to the data itself. So,
log.msg(format="Process %(executable)r wrote stderr unhandled by %(protocol)s: %(data)s", executable=..., protocol=fullyQualifiedName(protocol), data=data)
.
- We're trying to move towards "structured" logging. This means that any time you log something, it should probably look more like this:
twisted/internet/test/test_endpoints.py
- Some new trailing whitespace to clean up.
- Call
addCleanup
as soon as possible after doing the thing that's being cleaned up. Any exception that happens in between those two actions prevents the cleanup from being done. - Try removing each occurrence of
self.ep._wrappedPP
from the unit tests. Actually, try removing the_wrappedPP
attribute from the implementation of the endpoint entirely. You don't need it, and the tests will be better without it.
- I broke the implementation of
ProcessEndpoint
in r34893. Try changing the tests or adding new ones to uncover the brokenness and then fixing it.
Thanks!
comment:13 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | ashfall deleted |
Replying to exarkun:
twisted/internet/process.py
I think you mean twisted/internet/endpoints.py
. :)
Made the suggested changes. New build results.
comment:15 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to ashfall |
Thanks!
- twisted/internet/test/test_endpoints.py
- What is
ProcessEndpointsTestCase.test_errFlag
testing? - The order of events in
test_wrappedProtocol
doesn't make sense. Eitherwpp
ought to be looked up insidecheckPPWrapper
or the assertion should be made outside ofcheckPPWrapper
. Splitting up the two parts of that assertion doesn't benefit anything. WrappedIProtocolTests.test_makeConnection
ends up calling_WrapIProtocol.makeConnection
twice. It shouldn't.- There appears to be no unit test that covers the result of the Deferred returned by
ProcessEndpoint.connect
. WrappedIProtocolTests.test_processEnded
should useProcessTerminated
instead ofConnectionLost
. OnlyProcessEnded
andProcessTerminated
will ever be passed to theprocessEnded
callback.
- What is
- twisted/internet/endpoints.py
- There's a conflict here in the
__all__
section that needs to be resolved - Whitespace on the line importing
ProcessProtocol
needs to be fixed - There's a spurious whitespace change to the implementation of
StandardIOEndpoint.listen
- The
process
attribute of_ProcessEndpointTransport
should be private - The documentation for
errFlag
in_WrapIProtocol.__init__
still suggests it takes a boolean value _WrapIProtocol.childDataReceived
andprocessEnded
need docstrings- The
log.msg
call inchildDataReceived
should use theformat
keyword argument tolog.msg
(example in one of my above comments) processEnded
should useFailure.check
_ERRFLAG_VALUE
almost certainly needs to be public, otherwise it will be difficult for anyone to pass values from it toProcessEndpoint
. I suggest a name more likeStandardErrorBehavior
, with attributes likeLOG
andDROP
/IGNORE
.ProcessEndpoint.__init__
documents_spawnProcess
with@ivar
.@ivar
only belongs in class docstrings (where this belongs, since_spawnProcess
is not an__init__
parameter).- The use of try/except and
defer.execute
anddefer.succeed
inProcessEndpoint.connect
is problematic. Add a test for that function's behavior in an error case - for example, passobject()
as the executable to launch.connect
should return aDeferred
that fails, but it won't.
- There's a conflict here in the
comment:16 Changed 8 years ago by
Branch: | branches/child-processes-endpoint-4696-2 → branches/child-processes-endpoint-4696-3 |
---|
(In [35108]) Branching to 'child-processes-endpoint-4696-3'
comment:17 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | ashfall deleted |
Addressed the review points, fixed all but 2.11, as discussed on IRC.
comment:18 follow-up: 19 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to ashfall |
2.11 still needs to be addressed. Our conversation on IRC was about how you can't provoke the error by passing a bad value for executable
. That doesn't mean it's impossible to provoke the error. Passing a bad value in the arguments list will provoke it against a real reactor. Since the tests use fakes, you'll need a fake that can somehow raise an exception (this should be very simple, since the fake is completely under the control of the test).
comment:19 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | ashfall deleted |
Replying to exarkun:
2.11 still needs to be addressed. Our conversation on IRC was about how you can't provoke the error by passing a bad value for
executable
. That doesn't mean it's impossible to provoke the error. Passing a bad value in the arguments list will provoke it against a real reactor. Since the tests use fakes, you'll need a fake that can somehow raise an exception (this should be very simple, since the fake is completely under the control of the test).
I misinterpreted. Fixed now.
comment:20 Changed 8 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:21 follow-up: 24 Changed 8 years ago by
Owner: | Jean-Paul Calderone deleted |
---|---|
Status: | assigned → new |
Thanks! Here's a couple points:
- The branch deletes the TCP server endpoints from the
__all__
definition fortwisted.internet.endpoints
. - Missing test coverage for
_WrapIProtocol.makeConnection
Can't finish this right now, unfortunately.
comment:22 Changed 8 years ago by
Owner: | set to ashfall |
---|
comment:23 Changed 8 years ago by
Branch: | branches/child-processes-endpoint-4696-3 → branches/child-processes-endpoint-4696-4 |
---|
(In [35758]) Branching to 'child-processes-endpoint-4696-4'
comment:24 follow-up: 26 Changed 8 years ago by
Owner: | ashfall deleted |
---|
Replying to exarkun:
Thanks for the review.
Thanks! Here's a couple points:
- The branch deletes the TCP server endpoints from the
__all__
definition fortwisted.internet.endpoints
.
Fixed in the new branch.
- Missing test coverage for
_WrapIProtocol.makeConnection
I think I messed something up while fixing 1.3 from your previous review in twisted.internet.WrappedIProtocolTests.test_makeConnection.
Something's giving me a false positive in this test, or leading me to believe makeConnection is called, when in fact it isn't?
comment:25 Changed 8 years ago by
Owner: | set to ashfall |
---|
comment:26 Changed 8 years ago by
Owner: | ashfall deleted |
---|
Replying to ashfall:
Replying to exarkun:
Thanks for the review.
Thanks! Here's a couple points:
- The branch deletes the TCP server endpoints from the
__all__
definition fortwisted.internet.endpoints
.Fixed in the new branch.
- Missing test coverage for
_WrapIProtocol.makeConnection
I think I messed something up while fixing 1.3 from your previous review in twisted.internet.WrappedIProtocolTests.test_makeConnection.
Something's giving me a false positive in this test, or leading me to believe makeConnection is called, when in fact it isn't?
So fixed this, but I still think something's not right with test_makeConnection. It passed even when makeConnection wasn't being called. Need to figure out _what_.
comment:27 Changed 8 years ago by
Once #6236 is resolved, I hope there will be less tedious subversion junk in the way of reviewing this.
comment:28 Changed 8 years ago by
Author: | ashfall → exarkun, ashfall |
---|---|
Branch: | branches/child-processes-endpoint-4696-4 → branches/child-processes-endpoint-4696-5 |
(In [36704]) Branching to 'child-processes-endpoint-4696-5'
comment:29 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to ashfall |
Hello again! With #6236 resolved, there were indeed many fewer merge conflicts. There were still a couple simple ones, so I merged the branch forward once more and resolved them myself. I also switched a couple uses of implements
to implementer
in r36706.
Some more comments about the code:
- There are a few more Python 3 compatibility changes necessary. These are mostly trivial (though unfortunately tedious, sorry):
twisted.internet.endpoints
may not import any modules that haven't been ported to Python 3 (sincetwisted.internet.endpoints
itself is expected to work on Python 3 now). The easiest solution to this is to take the new imports oftwisted.python.constants
and move them into theif not _PY3:
block. Since there are module-scope uses of the names imported fromtwisted.python.constants
, you probably also need to define some dummy versions in an else block for theif not _PY3:
. SinceStandardErrorBehavior
will get deleted when_PY3
is true, these dummy versions don't need to work, they just need to let the code get past the definition ofStandardErrorBehavior
. Collectively, this is the approach where you do not make this new functionality available on Python 3, and so you also do not add"ProcessEndpoint"
or the other related names to__all3__
. The harder solution is to actually makeProcessEndpoint
work on Python 3 which involves first porting its dependency,twisted.python.constants
. I recommend saving this task for later, after this ticket has been resolved.- Since the unit tests won't pass if
ProcessEndpoint
is deleted, at the end oftest_endpoints.py
you need to add the test cases being added by this branch to the delete statement, so they won't run on Python 3.
- There are a few pyflakes errors in
twisted/internet/test/test_endpoints.py
. - There are some boring doc/formatting issues. See r36707 where I fixed them (easier to explain them that way than here)
StandardErrorBehaviors
should document its attributes in its class docstring (@cvar
)MemoryProcessReactor.spawnProcess
fails to instantiateMemoryProcessTransport
. It just uses the class. This seems not to cause any tests to fail because the transport is never actually used whenMemoryProcessReactor
is used to set it up._ProcessEndpointTransport
is already tested byProcessEndpointTransportTests
so it's probably fine that the transport set up byMemoryProcessReactor.spawnProcess
is never actually used. I suggest usingobject()
as the transport, though, to avoid giving the impression that this code is actually exercised.
I forced a build on the new branch, <http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/child-processes-endpoint-4696-5>, that will point out a few other issues to take care of (don't worry too much about osx10.6-py2.6-select, it seems to be running on failing hardware). Do note the new twistedchecker builder, though, which checks for compliance with various (mostly stylistic) coding standard points.
Thanks.
comment:30 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | ashfall deleted |
Thanks! Fixed, new builds.
There's one twistedchecker error that looks like a bug in twistedchecker.
comment:31 Changed 8 years ago by
Owner: | set to Tom Prince |
---|
comment:32 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Tom Prince to ashfall |
Sorry for the delay in getting to this, and thanks for your persistence.
ProcessEndpointsTestCase
- It isn't clear that
test_errFlag
is testing anything useful.test_constructorDefaults
already tests thatself.ep._errFlag
is a specific constant inStandardErrorBehavior
. So this test can probably be deleted. - Since you already have a fake
IReactorProcess
, you could just have that store the arguments passed tospawnProccess
to test intest_spawnProcess
, rather than having_spawnProcess
which is only used in tests.
- It isn't clear that
ProcessEndpointTransportTests
- The docstrings for
test_getHost
andtest_getPeer
don't describe what the test actually asserts. Also, as written, the functions on_ProcessEndpointTransport
could call the reverse functions on the protocol, and the tests wouldn't notice.
- The docstrings for
WrappedIProtocolTests
test_logStderr
should probably check the format as well (otherwise, it could be removed, which would mean that by default nothing gets written to a log file).- The docstring for test_processEnded` is no longer accurate.
Please resubmit for review after addressing the above. One additional point that you may want to consider:
- There is now good support for testing things using deferreds without returning them from test methods. Specifically
successResultOf
,failureResultOf
, andassertNoResult
. Since you are using an in-memory reactor for everything, the tests could be improved by using these throughout.
comment:33 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | ashfall deleted |
Things fixed, thanks!
comment:34 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
A couple of minor nits:
ProcessEndpointsTestCase.test_processAddress
: Having an accessorgetAddress
seems gratuitous.- Rather than accessing
self.ep._reactor
, we should save the reactor on self. - What happens if
log.msg
is called in tests? Should it fail (in_logMsg
)?
Please merge after addressing the above points.
comment:35 Changed 8 years ago by
Actually, ProcessAddress
is going to be public, and I think it eventually wants to have more attributes (and thus more constructor arguments). So that should be done before merging.
For the peer address, I think it wants at least the executable and arguments. And perhaps also the pid. I'm not sure about for the host address. Probably the pid, and probably a different type.
comment:36 follow-up: 37 Changed 8 years ago by
Actually, ProcessAddress is going to be public, and I think it eventually wants to have more attributes (and thus more constructor arguments). So that should be done before merging.
Or it could be private for now and we can fix this later.
comment:37 Changed 8 years ago by
Replying to exarkun:
Actually, ProcessAddress is going to be public, and I think it eventually wants to have more attributes (and thus more constructor arguments). So that should be done before merging.
Or it could be private for now and we can fix this later.
+1. While it's important for the transport to satisfy its interface and return an IAddress
of some kind, I can't think of any reason application code would particularly want to construct these. Making it public later is easy.
comment:38 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:39 Changed 8 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:40 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
Status: | reopened → new |
Sorry about that. It seems to work on Python 3 though, so I fixed the imports.
Build results here: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/child-processes-endpoint-4696-5
comment:41 follow-up: 43 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
twisted on python3 doesn't support processes so I don't think having the endpoint is useful, or warranted.
comment:42 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
Fair enough, it should be fixed now.
comment:43 Changed 8 years ago by
Replying to tom.prince:
twisted on python3 doesn't support processes so I don't think having the endpoint is useful, or warranted.
It might not be useful, but it's warranted because we shouldn't be creating additional work for future porters. It might be OK to defer the porting work to the point where processes get ported, but if we can avoid that without putting in too much effort, then let's.
comment:44 Changed 8 years ago by
Glancing over the build results, it looks like "new pydoctor errors" builder still doesn't work right. These don't look like errors relevant to this branch: http://buildbot.twistedmatrix.com/builders/documentation/builds/3139/steps/api-documentation/logs/new%20pydoctor%20errors
comment:45 Changed 8 years ago by
Author: | exarkun, ashfall → exarkun, ashfall, tomprince |
---|---|
Branch: | branches/child-processes-endpoint-4696-5 → branches/child-processes-endpoint-4696-6 |
(In [38049]) Branching to child-processes-endpoint-4696-6.
comment:47 Changed 8 years ago by
Owner: | set to therve |
---|
glyph: StandardErrorBehavior
uses twisted.python.constants
which isn't ported.
This looks good. Please merge.
comment:48 Changed 8 years ago by
Keywords: | review removed |
---|
comment:49 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [34639]) Branching to 'child-processes-endpoint-4696'