Python 2to3, too #1227

grahamc · 2020-02-21T15:15:40Z

Continues #1214 by @Taneb. The result of running 2to3 on the codebase, plus manual fixes when we're dealing with bytes and strings... plus a few minor 2-to-3 incompatibilities like Queue.Queue and output flushing.

I have tested this deploying to machines which are deployed without any plugin, as well as machines deployed with the nixops-packet plugin, using:

this branch of nixops-packet input-output-hk/nixops-packet@master...grahamc:py3 (only the last 3 commits are relevant)
a recent nixpkgs-unstable at b94c1c89f69563a9fc2ceee487b9bc19e5234d6a
and this Nixpkgs overlay:

    (final: super: {
      nixopsBootstrap = (import "${../../../github.com/NixOS/nixops}/release.nix" {
        nixpkgs = final.path;
        p = (nixopsPlugins: [
        ]);
      }).build."${system}";

      nixops-packet = (import "${../../../github.com/input-output-hk/nixops-packet}/release.nix" {
        nixopsSrc = {
          outPath = ../../../github.com/NixOS/nixops;
          revCount = 0;
          shortRev = "abcdef";
          rev = "HEAD";
        };
        nixpkgs = final.path;
        nixops = final.nixopsBootstrap;
      });

      nixops = (import "${../../../github.com/NixOS/nixops}/release.nix" {
        nixpkgs = final.path;
        p = (nixopsPlugins: [
          final.nixops-packet
        ]);
      }).build."${system}";
    })

I have tested these things:

Creating a new system
Destroying an unneeded system
nixops ssh-for-each
nixops deploy
nixops deploy --force-reboot --exclude foo bar
nixops deploy -k
nixops deploy --include foo bar
nixops info
nixops list
nixops check
nixops send-keys
nixops show-arguments (but I had none)
nixops show-physical
nixops ssh (both to and from)
nixops dump-nix-paths
nixops export

I fixed many problems with combining bytes and strings in the logger, and bytes and strings when running remote commands. I fixed the stdout handler which had trouble with b"" != "" especially when detecting a remote SSH command was completed writing. I fixed the logger not flushing after each . write, and not flushing when asking if destroying things is okay.

IMO this is ready to go, and the upgrades to other plugins will need to be done after this merges.

AmineChikhaoui · 2020-02-21T15:33:31Z

had to modify script/nixops to have python3 in the shebang.
And got this error later:

  File "/home/amine/src/nixops/nixops/deployment.py", line 741, in worker
    res = m.switch_to_configuration(switch_method, sync)
  File "/home/amine/src/nixops/nixops/backends/none.py", line 65, in switch_to_configuration
    res = super(NoneState, self).switch_to_configuration(method, sync, command)
  File "/home/amine/src/nixops/nixops/backends/__init__.py", line 351, in switch_to_configuration
    return self.run_command(cmd, check=False)
  File "/home/amine/src/nixops/nixops/backends/__init__.py", line 337, in run_command
    return self.ssh.run_command(command, self.get_ssh_flags(), **kwargs)
  File "/home/amine/src/nixops/nixops/ssh_util.py", line 290, in run_command
    return nixops.util.logged_exec(cmd, self._logger, **kwargs)
  File "/home/amine/src/nixops/nixops/util.py", line 136, in logged_exec
    logger.log_start(data[start:])
  File "/home/amine/src/nixops/nixops/logger.py", line 145, in log_start
    self.main_logger.log_start(self._log_prefix, msg)
  File "/home/amine/src/nixops/nixops/logger.py", line 40, in log_start
    self._log_file.write(msg)
  File "/home/amine/src/nixops/nixops/util.py", line 293, in write
    self.stderr.write(data)
TypeError: write() argument must be str, not bytes
```

grahamc · 2020-02-21T15:35:55Z

🤡 I never ran it like that in dev mode, I used the nix-build't version 😬 I'll take another crack.

multun

some nits

maintainers/dump-route53-hosted-zone.py

nixops/backends/__init__.py

nixops/diff.py

nixops/nix_expr.py

nixops/deployment.py

grahamc · 2020-02-21T16:09:14Z

@multun all of those were added by 2to3. I'm a bit spooked about making those changes -- but trust your expertise. Would you mind sending my branch a PR?

multun · 2020-02-21T16:11:20Z

yeah sure ! I should probably have figured that out instead of making way too many comments, sorry about that

grahamc · 2020-02-21T16:18:51Z

No worries :D I appreciate you taking the time for it. Ping me here when you've opened that PR?

grahamc · 2020-02-21T16:30:39Z

@AmineChikhaoui I had to change the release.nix to create a nixops without any plugins, pass that to the plugins for testing, and then create a nixops combining them all together. Then I updated scripts/nixops to use python3 and verified it works ... can you try again?

AmineChikhaoui · 2020-02-21T17:22:03Z

@grahamc thanks seems to be working now !

grahamc · 2020-02-22T16:04:33Z

I rebased this on to the reformatted master, since reformatting & a mechanical 2->3 operation is probably a good thing to bulk together.

This test assumes peculiar ordering guarantees which have changed from python2. This switches it to be the output from python3, but a proper fix would either to test equality only up to permutation, or to fix and guarantee an actual ordering.

…3 them

…ey can doCheck

…cluded

talyz

Great stuff! Seems to be working well! I tried running without any plugins, only using deployment.targetHost and could run create, deploy, ssh, info and export without any issue.

dhess · 2020-02-26T17:26:39Z

I've tested this with 4 of my own deployments (mainly deploy) and it's working great for me.

grahamc · 2020-02-27T19:23:19Z

Note, other people can test this PR too. If you only use the none backend, you can simply:

nix-build ./release.nix -A build.x86_64-linux

and use the resulting ./result/bin/nixops.

grahamc · 2020-02-27T19:39:09Z

btw I'm planning on merging this tomorrow.

The last version wasn't correct. Added some types and validated with mypy.

aanderse

I'm not approving the code (which I haven't looked at), but I'm approving that my usual nixops workflow is fine with this build.

Thanks for your work @grahamc! 🎉

elseym · 2020-02-28T16:30:10Z

just tested this on darwin, got the following error:

building all machine configurations...
Traceback (most recent call last):
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/bin/.nixops-wrapped", line 251, in <module>
    args.op(args)
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/script_defs.py", line 571, in op_deploy
    max_concurrent_activate=args.max_concurrent_activate,
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 1347, in deploy
    self.run_with_notify("deploy", lambda: self._deploy(**kwargs))
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 1336, in run_with_notify
    f()
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 1347, in <lambda>
    self.run_with_notify("deploy", lambda: self._deploy(**kwargs))
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 1267, in _deploy
    dry_run=dry_run, repair=repair, include=include, exclude=exclude
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/deployment.py", line 772, in build_configs
    key_file = m.get_ssh_private_key_file()
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/backends/none.py", line 93, in get_ssh_private_key_file
    return self.write_ssh_private_key(self._ssh_private_key)
  File "/nix/store/3jp88jwfpy4nabx8fwh970mpdacihp0l-nixops-1.8pre0_abcdef/lib/python3.7/site-packages/nixops/backends/__init__.py", line 375, in write_ssh_private_key
    f.write(private_key)
TypeError: write() argument must be str, not None

we suspect that this is because not all hosts in the deployment are in the state file yet.

/cc @lheckemann

lheckemann · 2020-02-28T18:00:26Z

we suspect that this is because not all hosts in the deployment are in the state file yet.

we just realised: this is correct but has nothing to do with the python3 changes and happens with --build-only in the python2 version if the hosts have never been deployed before as well.

grahamc requested review from rbvermaa, edolstra and AmineChikhaoui and removed request for rbvermaa and edolstra February 21, 2020 15:18

multun reviewed Feb 21, 2020

View reviewed changes

AmineChikhaoui approved these changes Feb 21, 2020

View reviewed changes

grahamc force-pushed the python-2to3-too branch from 005ba0e to fc4e6f7 Compare February 22, 2020 16:03

Nathan van Doorn and others added 14 commits February 22, 2020 17:03

Upgrade to Python3

8ee1b1e

Change unit test target

f79a59e

This test assumes peculiar ordering guarantees which have changed from python2. This switches it to be the output from python3, but a proper fix would either to test equality only up to permutation, or to fix and guarantee an actual ordering.

Finalize python3 upgrade

fa12093

2to3: undo the changes- to deployment.py and script_devs.py, then 2to…

5e1f0c5

…3 them

parallel.py: fixup Queue.Queue to queue.Queue

29e60a2

check_output calls: decode to strings before using

42f037c

util.py: logged_exec: handle output as bytes, but return a string

ad3630c

TeeStdout: handle bytes and strings

9aac080

logger: handle bytes and strings

e56f1f0

logger.py: flush after each log message

dda6061

release.nix: build a bootstrapping nixops to pass in to plugins so th…

15b4cba

…ey can doCheck

scripts/nixops: use python3

d3d26f0

treewide: improve 2to3 conversion

a6e1810

treewide: s/python2/python3/g

50f12e4

grahamc added 2 commits February 22, 2020 17:04

logger: fixup another string / byte confusion

dd5fb9f

nixops: make the op required

eeac976

grahamc force-pushed the python-2to3-too branch from fc4e6f7 to eeac976 Compare February 22, 2020 22:05

nixops ssh-for-each: handle None's from if a machine is excluded / in…

f04bdb5

…cluded

talyz approved these changes Feb 23, 2020

View reviewed changes

nixops info: None isn't sortable

42c39eb

grahamc added 4 commits February 28, 2020 00:05

deployment: drop statefile import, unused

967694f

nixops info: None isn't sortable, try 2

ab7c3f4

The last version wasn't correct. Added some types and validated with mypy.

travis: check mypy

840ba01

add in a few more types ...

e8e0a22

aanderse approved these changes Feb 28, 2020

View reviewed changes

grahamc mentioned this pull request Feb 28, 2020

2-to-3: Start with types #1232

Merged

grahamc closed this Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python 2to3, too #1227

Python 2to3, too #1227

grahamc commented Feb 21, 2020

AmineChikhaoui commented Feb 21, 2020

grahamc commented Feb 21, 2020

multun left a comment

grahamc commented Feb 21, 2020

multun commented Feb 21, 2020

grahamc commented Feb 21, 2020

grahamc commented Feb 21, 2020

AmineChikhaoui commented Feb 21, 2020

grahamc commented Feb 22, 2020

talyz left a comment

dhess commented Feb 26, 2020

grahamc commented Feb 27, 2020

grahamc commented Feb 27, 2020

aanderse left a comment

elseym commented Feb 28, 2020

lheckemann commented Feb 28, 2020

Python 2to3, too #1227

Python 2to3, too #1227

Conversation

grahamc commented Feb 21, 2020

AmineChikhaoui commented Feb 21, 2020

grahamc commented Feb 21, 2020

multun left a comment

Choose a reason for hiding this comment

grahamc commented Feb 21, 2020

multun commented Feb 21, 2020

grahamc commented Feb 21, 2020

grahamc commented Feb 21, 2020

AmineChikhaoui commented Feb 21, 2020

grahamc commented Feb 22, 2020

talyz left a comment

Choose a reason for hiding this comment

dhess commented Feb 26, 2020

grahamc commented Feb 27, 2020

grahamc commented Feb 27, 2020

aanderse left a comment

Choose a reason for hiding this comment

elseym commented Feb 28, 2020

lheckemann commented Feb 28, 2020