New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Python 3 support #16

Closed
dw opened this Issue Sep 8, 2017 · 22 comments

Comments

Projects
None yet
5 participants
@dw
Copy link
Owner

dw commented Sep 8, 2017

Too many changes to even consider just yet

@dw dw added the core label Sep 8, 2017

@href

This comment has been minimized.

Copy link

href commented Mar 6, 2018

Any chance of this being something that could be done in the light of your announced Kickstarter project? I'd love to integrate mitogen into Suitable but I'm just not willing to go back to Python 2 :)

@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Mar 6, 2018

We have at least a simple, solid plan, but so far I don't like it -- it would mean 3.x masters can't talk to 2.x children. Still a work in progress, I want to fix this without dropping 2.4 support. Ultimately it's probably simple work, but the testing involved is why I can't include it in the initial 3 weeks, which are already a stretch simply due to the testing work. But if you back the project I'll treat this as a priority and try squeeze it in for the 1 month mark :)

@href

This comment has been minimized.

Copy link

href commented Mar 6, 2018

Deal. Time to convince my boss to send some money your way :)

@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Mar 6, 2018

@href

This comment has been minimized.

Copy link

href commented Mar 7, 2018

Done. Seantis, my employer, should have made a pledge :)

@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Mar 7, 2018

@href

This comment has been minimized.

Copy link

href commented Mar 7, 2018

Our environment is mixed. Though 3.x to 3.x is probably okay, as long as we can specify the path to the Python binary.

I’m mainly interested in speeding up our Suitable code base and I need to support Python 2/3 there. So I expect we’ll use our support hours to have you help us out there :)

@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Mar 7, 2018

@href

This comment has been minimized.

Copy link

href commented Mar 7, 2018

You are welcome :)

@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Mar 12, 2018

Chatting about 3.x support, the Ansible guys pointed me to https://github.com/ansible/ansible/blob/80ba7b74022578e0685ea030e88821e87f28c520/lib/ansible/module_utils/pycompat24.py#L32 .. which solves the single major issue with having a single unified 2.x/3.x codebase that remains compatible all the way back to 2.4.

This hack only needs to happen in 8 places in the code, looks like we've finally got a solid plan for 2.x/3.x support :)

@moreati

This comment has been minimized.

Copy link
Contributor

moreati commented Mar 18, 2018

Some 2.x cPickle/pickle and 3.x _pickle/pickle differences I stumbled on, and wanted to make a note of

  1. In Python 2.x Subclassing Unpicklers
    1. cPickle.Unpickler requires replacing Unpickler.find_global(mod_name, class_name)
    2. pickle.Unpickler requires deriving a subclass that overrides Unpickler.load_global() with a method that does imports and fiddles with the unpickler stack

      If this sounds like a hack, you're right. Refer to the source code to make this work.

  2. In Python 3.x Restricting Globals
    1. pickle.Unpickler may be a pure Python implementation, or a C implementation from _pickle
    2. the method to override was renamed to find_class(mod_name, name) with the semantics of the old cPickle.Unpickler

dw added a commit that referenced this issue Apr 11, 2018

dw added a commit that referenced this issue Apr 17, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

dw added a commit that referenced this issue May 10, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.

dw added a commit that referenced this issue May 10, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.

dw added a commit that referenced this issue May 11, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.
@dw

This comment has been minimized.

Copy link
Owner Author

dw commented May 13, 2018

One more pickle difference: for protocol=2, Python 3 parses 2.x str as Unicode, and expects bytes to appear as a function call to codecs.encode(..., "latin1"). This hack allows 3.x to decode class instances from 2.x, where in 3.x instance attribute names must be Unicode, whereas in 2.x they were str.

As a general update on this, python3 branch is working relatively well already, including 2.x->3.x and 3.x->2.x. There is a bunch more work remaining, including a change to how module dependency scanning works. The bytecode format has changed at some point on 3.x, so we need tweaks to catch up.

Hoping to have the branch in a mergeable state sometime end of May.

@moreati

This comment has been minimized.

Copy link
Contributor

moreati commented May 13, 2018

One more 2.x vs 3.x difference with pickle. Python 3.x encodes integers outside the range -2**31 <= i <= 2**31-1 using opcodes for long() type. 64 bit builds of Python 2.x encode these integers using opcodes for int().

So the value 4294967296 (2**32) does not roundtrip from Python 2.x -> 3.x -> 2.x. It comes back out as 4294967296L. This probably an obscure quirk that can be ignored, but I wanted to document it.

@moreati

This comment has been minimized.

Copy link
Contributor

moreati commented May 16, 2018

4294967296 (2**32) does not roundtrip from Python 2.x -> 3.x -> 2.x. It comes back out as 4294967296L

On further thought, this is probably also true of Python 2.x 64-bit -> 2.x 32-bit -> 2.x 64-bit.

@href

This comment has been minimized.

Copy link

href commented May 24, 2018

I understand that Python 3 support is not yet completed. I've tried it however and wanted to make a note of this:

The ansible_mitogen plugin doesn't work at the moment, as it runs "import commands" in the mixins.py, which of course is not available in Python 3.

dw added a commit that referenced this issue Jun 10, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.

dw added a commit that referenced this issue Jun 10, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.

dw added a commit that referenced this issue Jun 10, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.

dw added a commit that referenced this issue Jun 11, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.

dw added a commit that referenced this issue Jun 11, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.
@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Jun 11, 2018

Python 3 Ansible host targetting Python 2.x child is now up to 173 steps of the integration tests. :)

I wouldn't even try it against a 3.x child yet -- there is definitely more work required. Meanwhile for 2.x targets, it is likely already working well for simpler playbooks. This shouldn't take much longer. Sorry for the delay!

Just to clarify, there are 4 cases that need to be supported:

  • 2.x host/target (already covered)
  • 3.x host, 2.x target
  • 3.x host, 3.x target
  • 2.x host, 3.x target

I think I've found most of the serialization-related issues, it's just down to ensuring string/Unicode types match up everywhere, and one 'featureful' sized issue to fix in the importer (issue #265).

@willmerae

This comment has been minimized.

Copy link

willmerae commented Jun 11, 2018

Victor Stinner (a core dev) spoke at PyLondinum yesterday, about the history of Python 3.x. Two slides caught my eye, as quirks/behaviour I didn't know about. Summarized below

Python 2.7 WONTFIX

  • ...
  • subprocess is not thread safe
  • threading.RLock is not signal safe
  • ...

Fixed in Python 3

  • ...
  • 3.4: file descriptors non-inheritable, fork+exec safety (PEP 446)
  • 3.5: retry syscalls on EINTR (PEP 475)

dw added a commit that referenced this issue Jun 11, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.

dw added a commit that referenced this issue Jun 11, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.

dw added a commit that referenced this issue Jun 13, 2018

issue #16: tests: 3.x fixes
* enable_debug_logging() must set _v/_vv globals.
* cStringIO does not exist in 3.x.
* Treat IOLogger and LogForwarder input as latin-1.
* Avoid ResourceWarnings in first stage by explicitly closing fps.
* Fix preamble_size.py syntax errors.
@RonnyPfannschmidt

This comment has been minimized.

Copy link

RonnyPfannschmidt commented Jul 1, 2018

as a note - execnet dropped pickle and implemented a own serializer to bridge the gap between py2 and py3

@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Jul 1, 2018

Hi Ronny :)

There are enough hacks to make pickle work in python3 branch -- the commits displayed in the GitHub UI no longer match the branch contents, as I've clearly been rebasing like crazy, and decided to just remove the issue references to stop spamming the ticket.

The branch is almost ready to merge - the built in tests pass for 2->3, 3->2, 3->3 and 2->2, however a big hang has been introduced that only manifests under the Ansible tests, and there is at least one hang on master. I need to find and fix both hangs before merging the result together.

Pickle definitely needs to go -- #126 is open for that, but it needs to stay at least for the first stable series, as that is now somewhere around 8 weeks behind schedule ;(

@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Jul 1, 2018

Just as a FYI if you're looking at this branch I will stop rebasing, and simply squash it prior to merge. I honestly thought it could be done in a single commit, which is how it's turned into such a disaster :P~

@RonnyPfannschmidt

This comment has been minimized.

Copy link

RonnyPfannschmidt commented Jul 1, 2018

so for now i wont touch it ^^

@dw dw closed this in d493a3d Jul 8, 2018

@dw

This comment has been minimized.

Copy link
Owner Author

dw commented Jul 8, 2018

This is now (finally) on master. A couple of inert races left that impact the unit tests around child process reaping, but not Ansible itself, so not delaying this any longer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment