Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Build: pass PATH environment variable to Docker container (#10133)" #10206

Closed
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 28 additions & 62 deletions readthedocs/doc_builder/environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from docker.errors import APIError as DockerAPIError
from docker.errors import DockerException
from docker.errors import NotFound as DockerNotFoundError
from requests.exceptions import ConnectionError, ReadTimeout # noqa
from requests.exceptions import ConnectionError, ReadTimeout
from requests_toolbelt.multipart.encoder import MultipartEncoder

from readthedocs.api.v2.client import api as api_v2
Expand Down Expand Up @@ -73,7 +73,7 @@ def __init__(
bin_path=None,
record_as_success=False,
demux=False,
**kwargs, # pylint: disable=unused-argument
**kwargs,
):
self.command = command
self.shell = shell
Expand Down Expand Up @@ -252,8 +252,8 @@ def save(self):
{key: str(value) for key, value in data.items()}
)
resource = api_v2.command
resp = resource._store["session"].post( # pylint: disable=protected-access
resource._store["base_url"] + "/", # pylint: disable=protected-access
resp = resource._store['session'].post(
resource._store['base_url'] + '/',
data=encoder,
headers={
'Content-Type': encoder.content_type,
Expand Down Expand Up @@ -301,58 +301,11 @@ def run(self):

self.start_time = datetime.utcnow()
client = self.build_env.get_client()

# Create a copy of the environment to update PATH variable
environment = self._environment.copy()
# Default PATH variable
# This default comes from our Docker image:
#
# $ docker run --user docs -it --rm readthedocs/build:ubuntu-22.04 /bin/bash
# docs@bfe702e31cdd:~$ echo $PATH
# /home/docs/.asdf/shims:/home/docs/.asdf/bin
# :/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
# docs@bfe702e31cdd:~$
#
# On old Docker images we have different PATH:
#
# $ sudo docker run -it readthedocs/build:latest /bin/bash
# docs@656e38a30fa4:/$ echo $PATH
# /home/docs/.pyenv/shims:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/home/docs/.conda/bin:/home/docs/.pyenv/bin
# docs@656e38a30fa4:/$
default_paths = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
if self.build_env.container_image in (
"readthedocs/build:ubuntu-22.04",
"readthedocs/build:ubuntu-20.04",
):
# Use ASDF path for newer images
python_paths = "/home/docs/.asdf/shims:/home/docs/.asdf/bin"
paths = f"{python_paths}:{default_paths}"

# On local development, we are using root user
if settings.RTD_DOCKER_COMPOSE:
paths = paths.replace("/home/docs/", "/root/")
else:
# Use PYENV for older images
paths = (
"/home/docs/.pyenv/shims:/home/docs/.cargo/bin"
f":{default_paths}:"
"/home/docs/.conda/bin:/home/docs/.pyenv/bin"
)
environment["PATH"] = paths

# Prepend the BIN_PATH if it's defined
if self.bin_path:
original_path = environment.get("PATH")
escaped_bin_path = self._escape_command(self.bin_path)
environment["PATH"] = escaped_bin_path
if original_path:
environment["PATH"] = f"{escaped_bin_path}:{original_path}"

try:
exec_cmd = client.exec_create(
container=self.build_env.container_id,
cmd=self.get_wrapped_command(),
environment=environment,
environment=self._environment,
user=self.user,
workdir=self.cwd,
stdout=True,
Expand Down Expand Up @@ -404,18 +357,31 @@ def get_wrapped_command(self):
"""
Wrap command in a shell and optionally escape special bash characters.

In order to set the current working path inside a docker container, we
need to wrap the command in a shell call manually.

Some characters will be interpreted as shell characters without
escaping, such as: ``pip install requests<0.8``. When passing
``escape_command=True`` in the init method this escapes a good majority
of those characters.
"""
prefix = ''
if self.bin_path:
bin_path = self._escape_command(self.bin_path)
prefix += f'PATH={bin_path}:$PATH '

command = (
' '.join(
self._escape_command(part) if self.escape_command else part
for part in self.command
)
)
return f"/bin/bash -c '{command}'"
return (
"/bin/sh -c '{prefix}{cmd}'".format(
prefix=prefix,
cmd=command,
)
)

def _escape_command(self, cmd):
r"""Escape the command by prefixing suspicious chars with `\`."""
Expand Down Expand Up @@ -558,14 +524,14 @@ class BuildEnvironment(BaseEnvironment):
"""

def __init__(
self,
project=None,
version=None,
build=None,
config=None,
environment=None,
record=True,
**kwargs, # pylint: disable=unused-argument
self,
project=None,
version=None,
build=None,
config=None,
environment=None,
record=True,
**kwargs,
):
super().__init__(project, environment)
self.version = version
Expand All @@ -591,7 +557,7 @@ def run(self, *cmd, **kwargs):
})
return super().run(*cmd, **kwargs)

def run_command_class(self, *cmd, **kwargs): # pylint: disable=signature-differs
def run_command_class(self, *cmd, **kwargs): # pylint: disable=arguments-differ
kwargs.update({
'build_env': self,
})
Expand Down