Opened 6 years ago

Last modified 5 years ago

#3722 enhancement new

Consolidate cred plugins into the core twisted dropin file

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/core-dropin-3722
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

Each project should only contribute one dropin to a particular plugin package.

Possibly also update the plugin documentation to point this out.

Change History (7)

comment:1 Changed 6 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/core-dropin-3722

(In [26668]) Branching to 'core-dropin-3722'

comment:2 Changed 6 years ago by exarkun

  • Owner changed from glyph to exarkun

comment:3 Changed 6 years ago by exarkun

I've moved all of the core plugins into a new core dropin and deleted all the existing core dropins. I've also moved a lot of strcred code out of the dropin and into either strcred.py or checkers.py. The strcred code is not fully tested, and there are some obvious shortcomings (eg the UNIXChecker's shadow support requires that your server be running as root).

I think this branch probably needs to add more test coverage for the strcred code.

comment:4 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Okay. I added a bit more test coverage for strcred. I really just wanted to get rid of uncovered lines, and that turned out to be pretty easy. The test coverage still isn't awesome, but I think it demonstrates that the code doesn't blow up with NameError or something stupid like that after all the renaming and such.

I was feeling a bit bad at all the extra modules which might get loaded now if twisted_core is imported (since before different dropins imported different parts of Twisted, and so you could get the ftp service maker plugin from the twisted_ftp dropin without importing the code defining the cred plugins). However, I checked how much worse this is in the branch than in trunk and it seems to be pretty bad in trunk already. In trunk, importing twisted.plugins.twisted_ftp results in 122 modules (94 more than you get in a clean Python process). In the branch, importing twisted.plugins.twisted_core results in 143 modules (115 more than in a clean process). So this is only trivially worse than it was. I'm not compelled to try to improve the situation by playing with imports. #3773 or #3774 can take care of this problem more generally.

One thing to consider with this change, though, is that it will interact very poorly with improper upgrades (upgrades which leave the old dropin files around, or even just their .pyc files). I can't think of anything to do about that, except continue to help people who don't know how to install or upgrade Python software learn.

comment:5 Changed 5 years ago by thijs

  • Cc thijs added
  • UNIXChecker, and possibly others, need an @since marker
  • copyright year updates

comment:6 Changed 5 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun

Thanks for working on this. Looks like there are some good changes here.

  1. I appreciate the sentiment behind the doc update, but "reasons of performance" is a really bad thing to put into a document. That's not to say that this information shouldn't be in the documentation, far from it; there should just be more than a single, unexplained, parenthetical comment. I would suggest putting the recommendation in two places: one which recommends it for reasons of code organization and (un-)installation; it's a lot easier to manipulate packages by hand if each distribution comes with only a single file to go in the "plugins" package. I suggest two new sections: one for "installation and removal of packages providing plugins", and one for "performance considerations". In the "performance considerations" section, we should explain which specific operations are slowed down by having multiple superfluous dropin files. This all might be a bit much to cram into this branch, but if it is, I'd rather just remove the documentation change completely. Littering the documentation with unexplained performance hints encourages users to develop systems which are tuned according to dogma rather than analysis.
  2. exarkun, your point about upgrades is well-taken. We need tools to help users cope with such things. Is there anything you could think of which would allow us to fail gracefully in this case? Even a specific API for declaring "Here are some plugins I used to have that I don't have any more, complain if they're present" would be nice. I think your recommendation for this branch makes sense - let's not address it here, and let's not block on it, because this is just something people need to start doing correctly when they work with Python code - but it would be nice to have a ticket to provide some tools to help users cope with plugin installation issues. Perhaps just a comment on #3865, if no specific, separate tool is necessary.
  3. Minor pyflakes nit: twisted/plugins/twisted_core.py:16: 'ICheckerFactory' imported but unused
  4. twisted.cred.checkers is definitely a much better home for that code than some random place in the plugin namespace. However...
    1. Shouldn't this be done in a separate branch? It seems like a completely separable piece of work, one which won't have any of the potentially negative consequences that need to be considered for the other changes here.
    2. Our development documentation talks about test namespaces as being unsupported by the compatibility policy, but it doesn't talk about plugins namespaces. I think that moving UNIXChecker with no deprecation is the right thing to do - depending on importing stuff from plugins is super dodgy - but the process documentation should say so.
    3. Just thinking out loud here: I think maybe UNIXChecker should go in its own module, rather than checkers, because it relies on platform APIs. It would be better to get a clean ImportError on Windows than wonky stuff happening when requestAvatarId gets invoked. An ImportError from an entire module strikes me as somehow cleaner than an individual class being conditionally missing (although both are better than "Here's that class you wanted, oops it doesn't actually work").
    4. This is an (exact?) duplication of code from twisted.conch.checkers.UNIXPasswordDatabase. #3242 vastly improves the testability and factoring of that code, so we should really get that landed before moving it around. There should be a ticket for removing this duplication.

Overall my recommendation would be to split this up and do the more non-problematic cleanup stuff in a separate phase before the actual plugin consolidation.

comment:7 Changed 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.