Opened 11 years ago
Closed 11 years ago
#21721 closed Bug (fixed)
Python 3.4 support
Reported by: | Marc Tamlyn | Owned by: | nobody |
---|---|---|---|
Component: | Python 3 | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | Florian Apolloner, berker.peksag@…, django@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Python 3.4 support should be added in master ready for 1.7, and probably also in 1.6.X as Py3.4 will be out before Django 1.7. As Py3.4 comes with bundled pip and venv, and is a prime target for new python developers and those running classes, it is very important for Django to support it.
I've run the test suite (on sqlite) against 1.6.X and master. A test run on 1.6.X can be seen at https://gist.github.com/mjtamlyn/8195542
The current state of things is as follows:
There are a couple of warnings printed at startup time. These relate to API changes in plistlib (which is OSX specific) and codecs (universal newlines reading of files has been deprecated).Our usage ofHTMLParser
should now always specify the value ofconvert_charrefs
as it's default will change in Py3.5. This is slightly problematic as Py2.7 does not have this kwarg so we can't universally apply it. Perhaps the best option is a six-like wrapper.django.utils.module_loading. module_has_submodule
has some issues with eggs.sys.meta_path
is giving usimportlib
as a finder.importlib.find_module
is deferred toimportlib.find_spec
(new in py3.4), which throws an error (ImportError: spec missing loader
).There is a failing test in the mail library regarding encoding in the mail module. Florian seemed to know about this.There is a significant issue with signal deregistration. I've been able to ascertain that something is up in thedjango.dispatch.saferef
module, but I don't have a sufficient understanding to work out what's wrong. There seems to be no test which tests this code directly and failures are thrown up at random during tests, test teardown and/or test suite teardown. They show up either asNoneType is not callable
or ascatching classes that do not inherit from BaseException is not allowed
.U
mode has been deprecated for files. We use it inmakemessages
and insql
, both for management commands. It seems unnecessary inmakemessages
, in thesql
we will have to be more careful as at present we split on\n
, which may need to be\r\n
on windows. However, it may well be fine anyway it probably doesn't matter if we have trailing\r
characters on the sql statements.- There are quite a lot of
ResourceWarning
s thrown for unclosed files(and sockets in the mail tests)
Attachments (1)
Change History (25)
comment:1 by , 11 years ago
comment:3 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 11 years ago
Cc: | added |
---|
Replying to mjtamlyn:
- There is a failing test in the mail library regarding encoding in the mail module. Florian seemed to know about this.
Not an issue, just use an up2date 3.4 (beta 2 at least!).
- There is a significant issue with signal deregistration. I've been able to ascertain that something is up in the
django.dispatch.saferef
module, but I don't have a sufficient understanding to work out what's wrong. There seems to be no test which tests this code directly and failures are thrown up at random during tests, test teardown and/or test suite teardown. They show up either asNoneType is not callable
or ascatching classes that do not inherit from BaseException is not allowed
.
No, it has nothing to do with saferef
, the main issue is that weakref
und onDelete
don't work properly on 3.4 during interpreter shutdown; to get this right you basically have to use finalize
: https://dpaste.de/ZeyD (That patch needs a little bit of cleanup but is otherwise ready to go)
comment:5 by , 11 years ago
Okay, my patch doesn't really work yet since it doesn't disconnect all receivers, gotta fix that *gg* http://bugs.python.org/issue19255 might allow us to use weakref's onDelete again.
by , 11 years ago
Attachment: | broken_builtins.diff added |
---|
comment:6 by , 11 years ago
The attached patch fixes the None is not callable
issues, TypeError: catching classes that do not inherit from BaseException is not allowed
still occurs with this variant.
comment:7 by , 11 years ago
That patch is still pretty much unstable, there appear to be a few issues with saferef
due to all the weakrefing :(
comment:8 by , 11 years ago
I've got it working: https://github.com/apollo13/django/commit/236a0ba02608a7c8cad9da20e76d049db11b12bd :)
comment:9 by , 11 years ago
And I got rid of saferef completely: https://github.com/apollo13/django/commit/7a6c28977bf258b27c5bc2a19417f913c541e2dd -- Please test as much as possible :)
comment:10 by , 11 years ago
I ran some tests with that commit. The good news is that python 3.4b2 seems to be fine - the only failures now are the egg loading related failures and (obviously) the tests for saferef. There were no nasty warnings in interpreter shutdown. A number of other warnings have also disappeared, although not when you run with -Wall
.
The bad news is that python 2.7 doesn't work very well. Basically all the tests in dispatch.tests
fail. https://gist.github.com/mjtamlyn/8304266. The same errors also happen on py3.3.
comment:11 by , 11 years ago
Oh, I just tested handlers+signals and had pyc files around :( Will rework my patch later on
comment:12 by , 11 years ago
Ok, that failure was just an oversight; fixed in https://github.com/apollo13/django/commit/8c43e774051494801be5ef576c6b0e5fb67ca60f
comment:14 by , 11 years ago
Any chance you can run djangobench for this? IIRC model_init_self_signals will benchmark signal sending.
comment:15 by , 11 years ago
Description: | modified (diff) |
---|
Signals branch now merged- thanks Florian.
I've opened a PR for the HTMLParser warnings - https://github.com/django/django/pull/2164
I've updated the description with the current outstanding issues. There are now only two test failures, relating to loading from eggs. Everything else is warnings.
comment:16 by , 11 years ago
I've done a draft approach for suppressing the ResourceWarning
s in core.mail
. We can use closing(get_connection())
in most cases in the tests and in Message.send()
, but we use a custom context manager in the main utilities as we may already have a connection
object. We do not want to close passed-in connections, that's the responsibility of whoever passed them in. Interestingly, the SMTP backend claims that just calling send_messages
on it will close itself. It seems that on python 3.4 at least it does not.
See diff at https://github.com/mjtamlyn/django/compare/mail-connection-wrappers. auto_connection
is a rubbish name, but it is a utility only needed in this module.
If we're happy with this approach, we should change the mail docs to recommend doing with closing(get_connection())
where get_connection()
is used, and add a note in the release notes that connections will now be closed. Ideally, I'd like to remove the implicit closing in send_messages
in the SMTP backend. It seems weird, but also unreliable as the implementation does not close the connection should the message fail.
In other news, I committed the HTMLParser
fix.
comment:17 by , 11 years ago
Description: | modified (diff) |
---|
Scrap that approach to fixing core.mail
. Turns out the open()
call on the SMTPBackend
was supposed to return True
if it opened a commit and it was not. The breaking change was only in place in 1.7. Fixed in https://github.com/django/django/commit/9d2c5b0. I've then also committed a few more careful closes where we explicitly open a connection in the tests.
Turns out the docs were right after all...
Description updated.
comment:18 by , 11 years ago
Description: | modified (diff) |
---|
The pdistlib
error is in fact a lack of dogfood in python itself: http://bugs.python.org/issue20229
comment:19 by , 11 years ago
@mjtamlyn: You can't just close the connection in https://github.com/mjtamlyn/django/compare/mail-connection-wrappers#diff-e1d718400701d0b0adfafbdcf8163aa3R284 -- if EmailMessage
was constructed with a valid connection it should stay open…
comment:20 by , 11 years ago
This approach turned out not be necessary. See comment 17. Python 3.4 caught a bug for us!
comment:21 by , 11 years ago
Description: | modified (diff) |
---|
comment:22 by , 11 years ago
Cc: | added |
---|
django.utils.module_loading. module_has_submodule has some issues with eggs. sys.meta_path is giving us importlib as a finder. importlib.find_module is deferred to importlib.find_spec (new in py3.4), which throws an error (ImportError: spec missing loader).
I didn't get any test failures with the current default branch of CPython 3.4. Probably related to http://bugs.python.org/issue20763.
comment:23 by , 11 years ago
Cc: | added |
---|
comment:24 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I can confirm that in python3.4 rc3 there are no test failures. There are still warnings from the deprecation of the imp
module, but that is tracked in #21628.
Loic pointed out on IRC a more significant issue with
module_loading
- which is that the code we use for acquiring a global import lock has also been deprecated. See http://docs.python.org/dev/library/imp.html#imp.acquire_lock