Dockerfiles Python 3 cleanup #3601
Conversation
Python 3.4 raised an error in Ubuntu14.04 image
|
Thanks. Python 3 is kinda annoying. :( |
| unzip \ | ||
| git \ | ||
| curl \ | ||
| tree \ | ||
| openjdk-11-jdk-headless | ||
|
|
||
| RUN update-alternatives --install /usr/bin/python python /usr/bin/python3 10 |
There was a problem hiding this comment.
is linking python to python3 needed - was it that transient dependency which needed it?
There was a problem hiding this comment.
I got this error:
ERROR: An error occurred during the fetch of repository 'pip_deps':
pip_import failed: (src/main/tools/process-wrapper-legacy.cc:75: "execvp(python, ...)": No such file or directory
)
ERROR: no such package '@pip_deps//': pip_import failed: (src/main/tools/process-wrapper-legacy.cc:75: "execvp(python, ...)": No such file or directory
)
There was a problem hiding this comment.
That makes it sound like bazel (that .cc is a part of it) is trying to run python on the system, rather than python3 for some reason, which would be a preexisting issue. Maybe it's because test --host_force_python=PY3 isn't in tools/bazel.rc?
There was a problem hiding this comment.
Just tested with the following, but it still fails:
test --genrule_strategy=standalone
test --host_force_python=PY3
test --ignore_unsupported_sandboxing
test --spawn_strategy=standalone
There was a problem hiding this comment.
Seems to be related to this line. Maybe we don't need it? Testing it now with it removed.
Line 398 in ee73293
There was a problem hiding this comment.
Looks like it's needed for some of the tar and docker rules.
| @@ -413,7 +413,7 @@ def main(): | |||
| env_map['AUTOMAKE'] = discover_tool('automake', 'Automake', 'AUTOMAKE', '1.9.6') | |||
| env_map['AUTOCONF'] = discover_tool('autoconf', 'Autoconf', 'AUTOCONF', '2.6.3') | |||
| env_map['MAKE'] = discover_tool('make', 'Make', 'MAKE', '3.81') | |||
| env_map['PYTHON3'] = discover_tool('python3', 'Python3', 'PYTHON3', '3.4') | |||
| env_map['PYTHON3'] = discover_tool('python3', 'Python3', 'PYTHON3', '3.5') | |||
|
|
|||
There was a problem hiding this comment.
this should probably check that python -m venv returns non-zero (the version will be same as the python interpreter)
There was a problem hiding this comment.
Do you have a suggestion of how best to do this? Seems the discover_tool method wouldn't work for what you describe, right?
There was a problem hiding this comment.
It might take a custom test, which I think there's already some of. I'm not sure if there is a nice portable test for venv other than something like:
import subprocess, tempfile
def test_venv():
with tempfile.TemporaryDirectory() as tmpdirname:
return subprocess.run(["python3", "-mvenv", tmdpirname]).returncode == 0The reason I think this is because I suspect there will be difference with how distribution maintainers can choose how this is included, and trying to cater for nuances doesn't sound good.
trivia: Debian make venv and pip separate packages, whereas the cpython codebase can let you install venv along with python, and then pip will be created using vendoered version that ensurepip unfurls (debian patches this out).
There was a problem hiding this comment.
I added the test function. Let me know if it looks good.
Co-authored-by: Oliver Bristow <evilumbrella+github@gmail.com>
There was a problem hiding this comment.
The changes to use make python→python3 ok to me. It's not risk free, as it will mean CI will not bump into issues developers would encounter if they have python→python2, but seems a manageable risk - on the bright side it could reduce the risk of issues in official builds.
It looks like the dead images are yet to be removed following your comment.
The python-venv change also seems ok, but does add an additional requirement to developer environments unlike using (an old) virtualenv in the WORKSPACE, so would be something to put some handrails on via ./configure_bazel.py.
| @@ -413,7 +413,7 @@ def main(): | |||
| env_map['AUTOMAKE'] = discover_tool('automake', 'Automake', 'AUTOMAKE', '1.9.6') | |||
| env_map['AUTOCONF'] = discover_tool('autoconf', 'Autoconf', 'AUTOCONF', '2.6.3') | |||
| env_map['MAKE'] = discover_tool('make', 'Make', 'MAKE', '3.81') | |||
| env_map['PYTHON3'] = discover_tool('python3', 'Python3', 'PYTHON3', '3.4') | |||
| env_map['PYTHON3'] = discover_tool('python3', 'Python3', 'PYTHON3', '3.5') | |||
|
|
|||
There was a problem hiding this comment.
It might take a custom test, which I think there's already some of. I'm not sure if there is a nice portable test for venv other than something like:
import subprocess, tempfile
def test_venv():
with tempfile.TemporaryDirectory() as tmpdirname:
return subprocess.run(["python3", "-mvenv", tmdpirname]).returncode == 0The reason I think this is because I suspect there will be difference with how distribution maintainers can choose how this is included, and trying to cater for nuances doesn't sound good.
trivia: Debian make venv and pip separate packages, whereas the cpython codebase can let you install venv along with python, and then pip will be created using vendoered version that ensurepip unfurls (debian patches this out).
| unzip \ | ||
| git \ | ||
| curl \ | ||
| tree \ | ||
| openjdk-11-jdk-headless | ||
|
|
||
| RUN update-alternatives --install /usr/bin/python python /usr/bin/python3 10 |
There was a problem hiding this comment.
That makes it sound like bazel (that .cc is a part of it) is trying to run python on the system, rather than python3 for some reason, which would be a preexisting issue. Maybe it's because test --host_force_python=PY3 isn't in tools/bazel.rc?
There was a problem hiding this comment.
Looking over I think @Code0x58 gave an in depth review. No issues from me.
No description provided.