Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#8123 enhancement closed fixed (fixed)

Deprecate twisted.runner RPCServer and RPCServicesConf

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/runner-rpcserver-deprecation-8123-3
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban, glyph

Description

From glyph

The parts that are totally broken, RPCServicesConf, RPCServer, and the code that instantiates it, are useful only to NFS server implementors, and don't work at all no matter your configuration. So I would delete the whole implementation just so someone will get a clear notification in case they were importing one of these names but not actually using them, deprecate the module attributes, and remove them in the next available removal cycle. So basically just leave an empty Service subclass there just as a courtesy (since that is slightly more polite, and only a tiny bit harder than just deleting it).

Change History (16)

comment:1 Changed 5 years ago by Adi Roiban

While on this, maybe I can also update the module description

The purpose of inetdtap is to provide an 'inetd'-like server, to allow Twisted to invoke other programs to handle incoming sockets. This is a useful thing as a "networking swiss army knife" tool, like netcat. In fact, it does work, after a fashion; I wrote a file like this:

8123       stream  tcp  wait glyph  /bin/cat -

and then ran this:

put a blank line into 'rpc.conf', and then ran:

twistd -n inetd -f sampleinetd.conf -r rpc.conf

and port 8123 properly became an echo server.

Now, in order to do this, you need a /etc/services file (this is hard-coded) that can be parsed by ServicesConf, which is _extremely_ picky, and probably the one that your OS comes with is still broken anyway. But this code can work if properly configured, and those parts can be fixed.

comment:2 Changed 5 years ago by Adi Roiban

Author: adiroiban
Branch: branches/runner-rpcserver-deprecation-8123

(In [46412]) Branching to runner-rpcserver-deprecation-8123.

comment:3 Changed 5 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I push the initial changes.

Test fail as I think that I need to work on improving the @deprecated decorator... rather than updating the way @deprecated is called in this branch.

I have also tried to document the twistd -n inetd command, but I could not find a way to inject more help info into twistd -n inetd --help command.

Please advise how to have the inetd usage info in the command line help.

I have manually tested the inetd and it works OK for echo server.

Thanks!

comment:4 Changed 4 years ago by hawkowl

This is blocked on #8124, it seems?

comment:5 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

This is failing tests (perhaps due to that dependency?) so pulling it back out of review for now. Otherwise it looks fine; hope we can land once the dependency is resolved.

comment:6 Changed 4 years ago by Glyph

Author: adiroibanadiroiban, glyph
Branch: branches/runner-rpcserver-deprecation-8123branches/runner-rpcserver-deprecation-8123-2

(In [47068]) Branching to runner-rpcserver-deprecation-8123-2.

comment:7 Changed 4 years ago by Glyph

I merged forward, but the tests are still failing. #8124 is now resolved though, so perhaps this could be fixed now. Adi?

comment:8 Changed 4 years ago by Adi Roiban

The branch is using the wrong deprecation helpers.

Also the advertised version is wrong.

I have pushed a change but a new commit is required for updating the tests.

I am not sure if I am using the right deprecation methods. Please review #8082

something is very broken on my system ... and is a bit late here

(py27-tests)$ ./bin/trial twisted/runner/inetdconf.py

-------------------------------------------------------------------------------

PASSED

comment:9 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

OK. Stupid error due to trying to program while tired :( .. I was not running the test files.


Test updated. Please review first the documentation talking about deprecation conventions.

This branch is ready for a final review.

Thanks!

comment:10 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Thanks for the update, Adi.

  1. it's 16.2.0 now :-\
  2. There are still failures on the buildbot; the one on Windows is real, although the others may be spurious.
  3. The feature doesn't actually work, so preserving the command-line flag doesn't really have any value. That explicitly isn't a supported compatibility surface in CompatibilityPolicy; you can just delete the entry in optParameters without deprecating first.

Otherwise it looks pretty good. However, given that the test failures seem to be persistent on this change I want to be extra careful, so please resubmit for review when these points are addressed and the builders are green. Hopefully the final review can be quick :).

comment:11 Changed 4 years ago by Adi Roiban

Branch: branches/runner-rpcserver-deprecation-8123-2branches/runner-rpcserver-deprecation-8123-3

(In [47282]) Branching to runner-rpcserver-deprecation-8123-3.

comment:12 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I think that the builder should be now green.

please review.

thanks!

comment:13 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Please fix the one outstanding twistedchecker error (I think by adding literally a single space) and then land :).

comment:14 Changed 4 years ago by Adi Roiban

twistedchecker is for this line and I think that this is a false positive.

    commentChar = '#'

will merge the branch as it is.

thanks for the review

comment:15 Changed 4 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [47323]) Merge runner-rpcserver-deprecation-8123-3: Deprecate twisted.runner RPCServer and RPCServicesConf.

Author: adiroiban Reviewer: glyph Fixes: #8123

comment:16 Changed 3 years ago by Glyph

Adding some Cc:'s, and:

How much longer until this (particularly, portmap.c) can die in a removal, now that it's deprecated?

Note: See TracTickets for help on using tickets.