Httplib2 py2/py3 shim and other py3 compat fixes

[Globegitter]

The comment here: #42 (comment) said hat we could "shim" over the py2/3 versions of httplib2 so that is what I tried to do. I am not sure if that is what was meant but from my local testing things seem to work.

I also had to fix the loading of the concurrent backport library and removed the seemingly unused oauth2client. Also the tests now get run with python3.

Fixes #42 and #97

[Globegitter]

Seems the tests are failing due the docker login command, unrelated to my changes.

[Globegitter]

@deft-code Any chance to get feedback on this?

[dmitrig01]

This is not working for me. It seems that the path is somehow misconfigured such that even the python3 import of concurrent.futures ends up importing concurrent_py2.concurrent.futures:

$ bazel run //:puller
INFO: Analysed target //:puller (2 packages loaded).
INFO: Found 1 target...
Target //:puller up-to-date:
  bazel-bin/puller
INFO: Elapsed time: 0.247s, Critical Path: 0.04s
INFO: 0 processes.
INFO: Build completed successfully, 4 total actions
INFO: Build completed successfully, 4 total actions
Traceback (most recent call last):
  File "execroot/containerregistry/bazel-out/darwin-fastbuild/bin/puller.runfiles/containerregistry/tools/fast_puller_.py", line 27, in <module>
    from containerregistry.client.v2 import docker_image as v2_image
  File "execroot/containerregistry/bazel-out/darwin-fastbuild/bin/puller.runfiles/containerregistry/client/v2/__init__.py", line 43, in <module>
    from containerregistry.client.v2 import docker_session_
  File "execroot/containerregistry/bazel-out/darwin-fastbuild/bin/puller.runfiles/containerregistry/client/v2/docker_session_.py", line 27, in <module>
    import concurrent.futures as concurrent_futures # <-- THIS LINE LOOKS RIGHT
  File "execroot/containerregistry/bazel-out/darwin-fastbuild/bin/puller.runfiles/concurrent_py2/concurrent/futures/__init__.py", line 8, in <module> # <--- This is the issue: it's getting concurrent/futures from py2, not py3
    from concurrent.futures._base import (FIRST_COMPLETED,
  File "execroot/containerregistry/bazel-out/darwin-fastbuild/bin/puller.runfiles/concurrent_py2/concurrent/futures/_base.py", line 414
    raise exception_type, self._exception, self._traceback
                        ^
SyntaxError: invalid syntax

I will investigate and try and fix shortly.

[dmitrig01]

Sorry, this is a bit beyond my current Bazel skills to fix in a clean way. Perhaps someone with more knowledge here can fix it.

[dmitrig01]

Hm, even once I get that to work, when I actually use the puller.par, another error, having to do with a string/bytes change in python3. Here's how I made it work in python3, though I again don't know the clean way to make it backwards-compatible:

diff --git a/client/docker_creds_.py b/client/docker_creds_.py
index e8d5a0f..435104d 100755
--- a/client/docker_creds_.py
+++ b/client/docker_creds_.py
@@ -156,8 +156,8 @@ class Helper(Basic):

     # Some keychains expect a scheme:
     # https://github.com/bazelbuild/rules_docker/issues/111
-    stdout = p.communicate(input='https://' + self._registry)[0]
-    if stdout.strip() == _MAGIC_NOT_FOUND_MESSAGE:
+    stdout = p.communicate(input=('https://' + self._registry).encode())[0]
+    if stdout.decode("utf-8").strip() == _MAGIC_NOT_FOUND_MESSAGE:
       # Use empty auth when no auth is found.
       logging.info('Credentials not found, falling back to anonymous auth.')
       return Anonymous().Get()

[Globegitter]

@dmitrig01 Thanks for testing it out and evaluating, let me have another look.

[Globegitter]

@dmitrig01 Oh yeah that is strange about the futures - how did you fix it for you?

Edit: Ok should have managed to fix both issues in a py2/3 compatible way.

[dmitrig01]

I just removed the concurrent_py2, because I don't need Python 2 to work :-). But as far as I could tell, the problem was as follows:
Directory structure is roughly this:

puller
puller.runfiles/
  concurrent_py2/
    concurrent/
      futures/
      __init__.py
  other_dependency/

The pythonpath has (at least) the following things on it:

[
  "puller.runfiles",
  "puller.runfiles/concurrent_py2",
  "puller.runfiles/other_depedency",
]

I believe what that means is that it's looking inside concurrent_py2, and seeing a package called concurrent - thus import concurrent is coming from there.

I think this is the issue, but it could be that my understanding of pythonpath isn't quite right either...

[Globegitter]

Yep the issue is related to: bazelbuild/bazel#5899 - but yeah will hopefully have a fix for that soon.

[dmitrig01]

ah nice, yes, that's the same issue

[dmitrig01]

I was thinking we could patch around it by having some file/directory renaming in build_file_content in the mean time -- I would assume that a fix to that issue will take a while to land. But not sure what the ethos of these projects is :-)

[Globegitter]

Yeah that is basically what I did now. It does work for me locally now but strangely enough I am getting an error when I try to copy puller.par into a docker container. How is this looking for you @dmitrig01 ?

[Globegitter]

Ok I think I figured out what the issue is and yeah it seems wee will need to create a new subdirectory so we can still maintain the concurrent directory at the bottom level.

[Globegitter]

There is also bazelbuild/bazel#6532 now, so we might just need to wait for a newer bazel version to have this fixed.

[JacobAMason]

For others experiencing this issue, the quick and dirty workaround is to export BRAZIL_PYTHON=/path/to/python2 after cleaning your workspace.

[whilp]

bazelbuild/bazel#6532 is coming along; perhaps this can be picked up soon.

[Globegitter]

Yes, also the whole work on better python2/3 mixing that should be in bazel 0.23 might be able to fix that. Let's see how it looks with the rc next week.

[randomvariable]

Is there anything we can do to accelerate Python 3 support?

Lack of support is giving lots of problems in kubernetes-sigs/cluster-api-provider-aws#624

[Globegitter]

@randomvariable now with bazel 0.23 out it is possible to only choose a dependency for py2 or py3, so this PR could be fixed and simplified. I am just not sure when I would get to it next.

[codebreach]

is this going to get merged anytime soon? lack of py3 support is preventing our monorepo from switching over to using bazel.

[Globegitter]

@codebreach sorry for just replying now, I dropped this as bazel has much better support for py2/3 now and because rules_docker will drop this containerregistry and already started switching over to the go version. Happy for anyone else to pick this up.

[c4urself]

Quite a few people are interested in this, it seems that Google is not making the Py2=>Py3 jump as quickly as most (or even at all) and this project is one of the victims. To make matters worse this project is not (actively) maintained: #114 (comment).

The following issue: bazelbuild/rules_docker#580 tracks the progress made on switching to a Go-based container registry in the main rules_docker project -- it seems that at this time they're not done yet but well on their way and could use some help migrating.

While that work is on-going it may be worthwhile to fork this project, fix Python-3 compatibility and ask rules_docker to use the fork as the source for the legacy Python tooling. I'll be taking a look at this in the following weeks and report back if at all successful.

[c4urself]

Ok I've created two forks that get seem to pass basic tests for me, will keep testing with it but would appreciate other people's input:

• https://github.com/c4urself/rules_docker (see: c4urself/rules_docker@860e165)
• https://github.com/c4urself/containerregistry (see: c4urself@c058734)

To test it, switch out the io_bazel_rules_docker http_archive with the following:

git_repository(
    name = "io_bazel_rules_docker",
    remote = "https://github.com/c4urself/rules_docker.git",
    commit = "860e165f1a5c36eaefd289040a64ac6c14eb552e",
)