Skip to content
This repository has been archived by the owner on Oct 2, 2023. It is now read-only.

Make rules_docker compatible with Python 3? #842

Closed
fedenusy opened this issue May 15, 2019 · 27 comments
Closed

Make rules_docker compatible with Python 3? #842

fedenusy opened this issue May 15, 2019 · 27 comments

Comments

@fedenusy
Copy link

fedenusy commented May 15, 2019

Hello! When running rules_docker using Python 3, I get the following error:

ERROR: Analysis of target '//samwell:samwell_img' failed; build aborted: no such package '@go_image_base//image': Pull command failed: Traceback (most recent call last):
  File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/private/var/tmp/_bazel_fedenusy/82761fbb391b03a602abc1324ff91dda/external/puller/file/downloaded/__main__.py", line 30, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "/private/var/tmp/_bazel_fedenusy/82761fbb391b03a602abc1324ff91dda/external/puller/file/downloaded/containerregistry/client/__init__.py", line 23, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "/private/var/tmp/_bazel_fedenusy/82761fbb391b03a602abc1324ff91dda/external/puller/file/downloaded/containerregistry/client/docker_creds_.py", line 31, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 963, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 906, in _find_spec
  File "<frozen importlib._bootstrap_external>", line 1280, in find_spec
  File "<frozen importlib._bootstrap_external>", line 1254, in _get_spec
  File "<frozen importlib._bootstrap_external>", line 1235, in _legacy_get_spec
  File "<frozen importlib._bootstrap>", line 441, in spec_from_loader
  File "<frozen importlib._bootstrap_external>", line 594, in spec_from_file_location
  File "/private/var/tmp/_bazel_fedenusy/82761fbb391b03a602abc1324ff91dda/external/puller/file/downloaded/httplib2/__init__.py", line 988
    raise socket.error, msg
                      ^
SyntaxError: invalid syntax

From seeing this, I'm assuming rules_docker was written for Python 2.

The workaround I found is to:

  • Set my machine's default python to python2
  • Set --incompatible_use_python_toolchains
  • Set --incompatible_py3_is_default=false

I think it should be simple to make rules_docker compatible with Python toolchains. We would just need to declare python_version = "PY2" somewhere within rules_docker... thoughts?

@fedenusy
Copy link
Author

fedenusy commented May 15, 2019

I looked around the rules_docker repo and it seems like every py_binary already declares python_version = "PY2"... so maybe it's not so simple?

@nlopezgi
Copy link
Contributor

you probably need to add host_force_python=PY2 as exemplified in the bazelrc file: https://github.com/bazelbuild/rules_docker/blob/master/.bazelrc#L20
please let me know if this works as many of these new py toolchain flags are untested

@fedenusy
Copy link
Author

fedenusy commented May 16, 2019

To be clear, rules_docker works just fine with the following:

  • Machine's default python set to python2
  • --incompatible_use_python_toolchains
  • --incompatible_py3_is_default=false

But since I need to declare the 2nd flag, I'm thinking rules_docker is incompatible with the new Python toolchains. If rules_docker were compatible, then I wouldn't need --incompatible_py3_is_default=false?

(The problem stems from the fact that internally we write Python 3. That flag forces us to declare python_version = "PY3" in all of our targets... we'd much rather keep the default at PY3 if at all possible.)

@fedenusy fedenusy changed the title Make rules_docker compatible with Bazel's Python toolchains Make rules_docker compatible with Python 3 May 16, 2019
@fedenusy fedenusy changed the title Make rules_docker compatible with Python 3 Make rules_docker compatible with Python 3? May 16, 2019
@nlopezgi
Copy link
Contributor

Please read bazelbuild/bazel#7899 in detail for the status of python toolchains (see lots of comments about rules_docker there).
The tl;dr is that with incompatible_use_python_toolchains rules_docker needs to run with host_force_python=PY2 as we depend on a library that is not compatible with PY3 (and which we don't control) and its not possible in Bazel (now or in the past, in any way) to have a host config for both PY2 and PY3 as part of the same build.

So, you should be able to keep the default to PY3 (as long as you are not using PY3 as host tools), but you will need to use the host_force_python=PY2, or set python_version = "PY3" in all your targets that are only compatible with PY3 (which, imo, is not a bad thing). Please let me know if you have more questions.

@nlopezgi
Copy link
Contributor

closing, please let me know if you'd like me to reopen for some reason

@qzmfranklin
Copy link

@fedenusy rules_docker can be patched to work with python3. I've managed it inside our team. Please let me know how much you are still interested in it.

@fedenusy
Copy link
Author

@qzmfranklin yes I'm interested! How'd you pull it off?

@jdelfino
Copy link

In my opinion, this issue should not be closed. Currently, rules_docker can only be used by forcing all host tools to run under py2. We have host tools in our build that need to run under py3, so specifying host_force_python=PY2 is not an acceptable workaround for us.

I understand that fixing this is not trivial, but python2 retires in 6 months, so it will need to be dealt with somehow, at some point soon, right?

@nlopezgi
Copy link
Contributor

nlopezgi commented Jun 13, 2019

@jdelfino
Thanks, for your comment
host_force_python=PY2 is indeed a bad workaround, and indeed it blocks use of host tools that require PY3, but its all we have from Bazel atm. Also, iiuc, this is not a regression, it has never been possible to have host tools that build with PY2 and PY3 to be used as part of the same Bazel build.

Also, yes, we are dealing with it (just in a different way, thus the issue being closed). We are currently working on migrating to use go containerregistry. Please keep track of PRs landing soon if you are interested in this.

@jdelfino
Copy link

Thanks @nlopezgi! I don't believe this is a regression. For anyone else that ends up here, I found #580, which appears to track the migration to go containerregistry.

@ittaiz
Copy link
Member

ittaiz commented Jun 19, 2019

@nlopezgi is this solely because rules_docker depends on python containerregistry which is not actively maintained? When rules_docker will move to go containerregistry will this be resolved as far as rules_docker is concerned?

@nlopezgi
Copy link
Contributor

Hi @ittaiz, yes this should be resolved once we migrate off python containerregistry

@mickael-carl
Copy link

What about keeping this open to track actually this problem, until the move to go containerregistry is complete? That seems more sensible than closing imho, as this issue is something that will pop up again until the change lands, and --host_force_python=PY2 is just a workaround as you point out.

@nlopezgi nlopezgi reopened this Jun 20, 2019
@nlopezgi
Copy link
Contributor

Reopening just to make it easier for folks to find this issue, but --host_force_python=PY2 is the only workaround until we have either a better solution from Bazel team (see bazelbuild/bazel#7899) or migration to go containerregistry lands

listx pushed a commit to listx/k8s-container-image-promoter that referenced this issue Jun 22, 2019
Also, make Bazel run with Python2 for rules_docker, because of
bazelbuild/rules_docker#842.
listx pushed a commit to listx/k8s-container-image-promoter that referenced this issue Jun 22, 2019
Also, make Bazel run with Python2 for rules_docker, because of
bazelbuild/rules_docker#842.
listx pushed a commit to listx/k8s-container-image-promoter that referenced this issue Jun 22, 2019
Also, make Bazel run with Python2 for rules_docker, because of
bazelbuild/rules_docker#842.
prestonvanloon added a commit to prysmaticlabs/prysm that referenced this issue Jun 24, 2019
prestonvanloon added a commit to prysmaticlabs/prysm that referenced this issue Jun 24, 2019
* Update com_github_prysmaticlabs_go_ssz commit hash to 0fdbce2

* Update io_bazel_rules_k8s commit hash to cddc035

* Update dependency build_bazel_rules_nodejs to v0.32.2

* Update dependency com_github_prometheus_common to v0.6.0

* Update dependency com_github_syndtr_goleveldb to v1

* revert ssz breakage

* Update libp2p

* update infra

* update infra

* specify node_modules

* add flag

* clarify in comment

* update rules_docker

* workarounds for python2. see: bazelbuild/rules_docker#842
@fejta
Copy link

fejta commented Jun 28, 2019

If rules_docker requires --host_force_python=PY2 please add that to https://github.com/bazelbuild/rules_docker#setup

@nlopezgi
Copy link
Contributor

nlopezgi commented Jul 3, 2019

thanks @fejta, added to Setup in #953.

@c4urself
Copy link

I've got a Python-3 compatible branch that works for basic things for me, could use some help testing, see: google/containerregistry#109 (comment)

@nlopezgi
Copy link
Contributor

Hi @c4urself ,
Thanks for working on this. The main challenge you will have getting these changes in is that py containerregistry is not really taking too many community contributions atm, iiuc. The solution we are working on for this is to migrate off py containerregistry. Please follow #580 for status of that effort.

@c4urself
Copy link

@nlopezgi -- understood, looking forward to the Golang stuff getting finished. I've got a working version of rules_docker under Python2 and Python3 by simply changing the containerregistry rules to a git_repository pointing to a forked branch: c4urself@860e165 -- how crazy would it be to start using that git_repository in the official rules_docker and giving people Python-3 compatibility while we wait for #580?

@nlopezgi
Copy link
Contributor

@nlopezgi -- understood, looking forward to the Golang stuff getting finished. I've got a working version of rules_docker under Python2 and Python3 by simply changing the containerregistry rules to a git_repository pointing to a forked branch: c4urself@860e165 -- how crazy would it be to start using that git_repository in the official rules_docker and giving people Python-3 compatibility while we wait for #580?

too crazy for me (plus there's technical reasons why that would be a nightmare for us to maintain). I really don't want to make this situation more complex by adding another source of dependency / variability. But thanks for the suggestion!

@fejta
Copy link

fejta commented Aug 22, 2019

Do we have an ETA for the golang conversion? Can we convince containerregistry to accept the contribution? It is definitely annoying to simultaneously default bazel to py3 while having core rules like rules_docker not support py3.

@nlopezgi
Copy link
Contributor

nlopezgi commented Aug 22, 2019

status has been updated in #580. I would love for containerregistry to take contributions, but it seems the team in charge has other properties atm. As for changing the bazel default, that's a Bazel 1.0 decision that is probably hard to roll back. We appreciate your patience and hope to finish the migration soon.

@aiuto
Copy link
Contributor

aiuto commented Aug 23, 2019

Less extreme idea.....
We work with the containerregistry team and c4urself to allow contributions to a "py3" branch of the that repo. The premise being

  • that someone from rules_docker or bazel takes responsibility for merges into that branch only. This mitigates their resposibilty for making sure submissions do not break existing users.
  • that we produce a single release
  • that this release is what rule_docker depends on for the N?-months until it switches to go-containerregistry

ender503 added a commit to DLTcollab/tangle-accelerator that referenced this issue Aug 23, 2019
Because the container_push rule from rules_docker is written in
Python and its depend on the Python 2 packages, we need to enforce the
Bazel to use Python 2 with this rule.

These settings can be removed after the rules_docker migrate this rule
with go.

See also:
 - https://github.com/bazelbuild/rules_docker#known-issues
 - bazelbuild/rules_docker#842
@c4urself
Copy link

Sure, let me know, happy to work with whoever, for now I'm going to use my Python-3 compatible fork of rules_docker, and by the way these rules are great, loving it so far.

@nlopezgi
Copy link
Contributor

Less extreme idea.....
We work with the containerregistry team and c4urself to allow contributions to a "py3" branch of the that repo. The premise being

  • that someone from rules_docker or bazel takes responsibility for merges into that branch only. This mitigates their resposibilty for making sure submissions do not break existing users.
  • that we produce a single release
  • that this release is what rule_docker depends on for the N?-months until it switches to go-containerregistry

The main issue we have that makes these alternatives challenging is that the SoT for containerregistry is an internal google repo, and rules_docker needs to maintain compliance with this internal version to support internal use cases (even though rules_docker SoT is github). Changing the dep in rules_docker to something else would mean, for us, that the automation we have set in place to sync our SoT (in github) with internal repos would likely require manual intervention at each change to make sure discrepancies between SoT and internal rules do not cause breakages. Moreover, since our SoT is github, our test coverage is mostly here, so breaking internal users unintentionally would be a common occurrence.

howjmay pushed a commit to howjmay/tangle-accelerator that referenced this issue Aug 26, 2019
Because the container_push rule from rules_docker is written in
Python and its depend on the Python 2 packages, we need to enforce the
Bazel to use Python 2 with this rule.

These settings can be removed after the rules_docker migrate this rule
with go.

See also:
 - https://github.com/bazelbuild/rules_docker#known-issues
 - bazelbuild/rules_docker#842
maraino pushed a commit to mikemaxey/cert-manager that referenced this issue Sep 25, 2019
See following issues for details
* bazelbuild/rules_docker#842
* bazelbuild/rules_docker#580
* bazelbuild/bazel#7899

Signed-off-by: stuart.warren <stuart.warren@ocado.com>
@smukherj1
Copy link
Collaborator

Most of the problems mentioned in this issue should now be fixed because rules_docker python binaries have been migrated to Go binaries that depend on go-containerregistry. See #580.

@fedenusy
Copy link
Author

🎉 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants