Port to Python 3 #278
Port to Python 3 #278
Conversation
there will be tons of pyflakes warnings from the python3 porting strategy
the branch has some python packaging fixes in nixpkgs that we need
also refactor tests.nix and default.nix so they share more of the environment setup which is now slightly more complex
They're all redundant on Python 3
There was a problem hiding this comment.
This looks good. I left a few comments and suggestions inline. Most of them are not blocking, but this is a significant enough change that I'd like to review any changes made.
I'll note that I only looked at the diff, and didn't try to find other places that needed to be changed. I also didn't changes of b"" vs u"" etc against the places being called, but mostly looked locally to verify that the changes appeared reasonable. I presume that anything significant will be caught by tests.
| @@ -1,17 +1,19 @@ | |||
| let | |||
| sources = import nix/sources.nix; | |||
| in | |||
| { pkgs ? import sources.release2105 {} | |||
| { pkgs ? import sources.release2111 { } | |||
There was a problem hiding this comment.
Given that we pass in release 2105 in PrivateStorageio, I don't think we should be making this change here (at least not without also testing 2105 in CI for the moment.
There was a problem hiding this comment.
I think it will be easier to advance to 21.11 in PrivateStorageio than to back this up to 21.05. There are some build failures in our dependencies on 21.05 that aren't interesting in any way once we're up to 21.11 and will be annoying to fix. Since ZKAPAuthorizer is supplying Tahoe-LAFS, the fact that everything is working on 21.11 seems like a pretty strong signal that upgrading PrivateStorageio to 21.11 (at least for Tahoe-LAFS/ZKAPAuthorizer) will be straightforward. Although it is true I haven't really tried it out yet. I could do that before committing to this upgrade to confirm that is going to be the easier path.
However, we still cannot be frozen (because of Failure.cleanFailure) and cannot use attrs exception support (because we want value-based comparison).
Co-authored-by: Tom Prince <[email protected]>
TempDir.join(unicode) returns unicode TempDir.join(bytes) raises an Exception FilePath(unicode) produces a text-mode FilePath StorageServer.__init__ raises an exception if passed bytes So we can just rely on self.tempdir.path being the correct type already and even if it weren't StorageServer would immediately tell us.
|
Thanks for the review @tomprince . I think I've addressed most of the points. For a few others, I've left some comments in reply to yours. I think this is just about ready for a second look when you get a chance. I see there are CI failures on macOS and Windows. Since they don't happen for the Nix build I guess they have to do with version skew in some dependency and hopefully adding some tighter version pins will deal with them. |
Pretty sure this is about Tahoe-LAFS 1.17.1 which took RIStorageServer off of StorageServer. So I changed the version pin to be exactly 1.17.0 for now. This seems unrelated to the porting. We didn't see it before because 1.17.1 was only released in the last day or two. |
There was a problem hiding this comment.
This looks good. I think there are just three minor things to change (an asBytesMode call to remove, use from __future__ import annotations instead of "" for type forward-reference, update the target-version for black and rerun).
Optionally, you can also remove the lint-python (in favour of just having one), but that could be a followup as well.
I started on this and quickly ran into problems where multiple conflicting instances of dependencies were showing up (autobahn, at least). It wasn't immediately obvious why this was happening so I'll leave it for later. I filed #284 |
Now that PrivateStorageio/ZKAPAuthorizer#278 has been merged
Fixes #276
This drops Python 2 support.