Bad ssl check

[ilia]

     if self.wanted:
            if not ssl.supported:
"twisted/words/protocols/jabber/xmlstream.py" 
line:326

should be

     if self.wanted:
            if not ssl or not ssl.supported:

To properly handle 'no ssl' case

[spiv]

Actually, I think it should be simply

if ssl is None:

instead of

if not ssl.supported:

The way ssl is imported will ensure it is None if it's not supported.

[Ralph Meijer]

Fixed in source:branches/xmlstream-ssl-import-2171. This time, I made sure tests pass both when OpenSSL is there, and when not. We really should have a buildbot slave with OpenSSL not installed.

Please review.

[Jean-Paul Calderone]

I'd like to see some work on #1795, since without a buildslave testing this configuration it is unlikely to remain working for very long. It's also of somewhat questionable value to have only /some/ parts of Twisted work without OpenSSL. As long as use is restricted to a narrow area, this might be workable, but who knows when you'll stumble on something that falls over.

[Ralph Meijer]

As stated, I would welcome such work.

Depending on the XML server configuration, TLS negotiation may not be required, so we can simply establish a connection if OpenSSL were not available. I thought it to be nice to make it work like that. The tests for the case that OpenSSL is not available at least work in the case it actually is available, faking its absense. I just made a small error in the implementation that is now corrected.

Or is your point that OpenSSL should simply be required for Twisted Core?

Also, do your comments affect the review of this ticket?

[ilia]

Maybe you mean the same, in my case, SSL is not needed twisted.words jabber connections. How to disable it?

[Jean-Paul Calderone]

Replying to ralphm:

As stated, I would welcome such work.

:)

Depending on the XML server configuration, TLS negotiation may not be required, so we can simply establish a connection if OpenSSL were not available. I thought it to be nice to make it work like that. The tests for the case that OpenSSL is not available at least work in the case it actually is available, faking its absense. I just made a small error in the implementation that is now corrected.

Quite so, but automated testing is needed to ensure it remains fixed.

Or is your point that OpenSSL should simply be required for Twisted Core?

Nah.

Also, do your comments affect the review of this ticket?

I haven't looked at the code in the branch. I'm just pointing out that without automated tests, few of our development procedures make sense.

[Jean-Paul Calderone]

Glancing at this ticket again, I remembered why ssl.supported exists. The change in this branch won't work reliably on Python 2.3, where it is possible to import twisted.internet.ssl even when OpenSSL is present.

So you will need to do both checks, or otherwise fix the detection code, in order to support Python 2.3 and later versions.

[Ralph Meijer]

Actually, I was already using the idiom as documented in t.i.ssl:

try:
    from twisted.internet import ssl
except ImportError:
    ssl = None
if ssl and not ssl.supported:
    ssl = None

However, I wasn't inspecting the ssl variable properly, which this change should fix.

[Jean-Paul Calderone]

Change looks okay, except SSL is an acronym; all lower-case is okay sometimes, but Ssl is weird. Should be changed to SSL in savedSsl.

Not exactly sure what the scope of this ticket is meant to be. I suppose it is to make the jabber tests pass in the absence of OpenSSL, but since the jabber tests are mixed in with other words tests and running them in isolation is hard, this goal underwhelms, since test_msn fails if OpenSSL isn't present.

Also, an only slightly related point: importing OpenSSL to get TLSv1_METHOD is unnecessary, since TLSv1_METHOD is the default for CertificateOptions. Changing this doesn't quite do enough to allow testWantedSupported to pass without OpenSSL, but it should be possible to to provide enough of the CertificateOptions and TLS APIs so that it could pass without OpenSSL. What the point of this would be, I'm not sure, since you can't actually use this code without OpenSSL, so it doesn't really matter if the tests pass or run.

Anyhow, just fix the capitalization issue and then the branch is fine to merge, unless you feel like fixing test_msn as well (then put it up for review again afterwards).

[Jean-Paul Calderone]

Fixed by r18506