Skip to content

Commit

Permalink
Build: pass PATH environment variable to Docker container
Browse files Browse the repository at this point in the history
Instead of prepending all the commands with the `PATH=` variable, we pass the
environment variable directly to the Docker container.

This allow us to run multi-line shell script without failing with weird syntax
errors. Besides, the implementation is cleaner since all the environment
variables are passed to the commands in the same way.

I added some _default paths_ that I found by checking the local Docker
container. I'm also passing the users' path, depending if we are working locally
as root or in production.

This is not 100% complete and there may be some other issues that I'm not seeing
yet, but I think it's a first step to behave in a way our users are expecting.

Closes #10103
  • Loading branch information
humitos committed Mar 9, 2023
1 parent dd07a3e commit a596512
Showing 1 changed file with 40 additions and 29 deletions.
69 changes: 40 additions & 29 deletions readthedocs/doc_builder/environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
import structlog
from django.conf import settings
from django.utils.translation import gettext_lazy as _
from docker import APIClient
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
from requests.exceptions import ConnectionError, ReadTimeout # noqa
from requests_toolbelt.multipart.encoder import MultipartEncoder

from docker import APIClient
from readthedocs.api.v2.client import api as api_v2
from readthedocs.builds.models import BuildCommandResultMixin
from readthedocs.core.utils import slugify
Expand Down Expand Up @@ -73,7 +73,7 @@ def __init__(
bin_path=None,
record_as_success=False,
demux=False,
**kwargs,
**kwargs, # pylint: disable=unused-argument
):
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(
resource._store['base_url'] + '/',
resp = resource._store["session"].post( # pylint: disable=protected-access
resource._store["base_url"] + "/", # pylint: disable=protected-access
data=encoder,
headers={
'Content-Type': encoder.content_type,
Expand Down Expand Up @@ -301,11 +301,35 @@ 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
environment[
"PATH"
] = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
# Add asdf extra paths
environment["PATH"] += (
":/home/{settings.RTD_DOCKER_USER}/.asdf/shims"
":/home/{settings.RTD_DOCKER_USER}/.asdf/bin"
)

if settings.RTD_DOCKER_COMPOSE:
environment["PATH"] += ":/root/.asdf/shims:/root/.asdf/bin"

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

try:
exec_cmd = client.exec_create(
container=self.build_env.container_id,
cmd=self.get_wrapped_command(),
environment=self._environment,
environment=environment,
user=self.user,
workdir=self.cwd,
stdout=True,
Expand Down Expand Up @@ -357,31 +381,18 @@ 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 (
"/bin/sh -c '{prefix}{cmd}'".format(
prefix=prefix,
cmd=command,
)
)
return f"/bin/bash -c '{command}'"

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

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

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

0 comments on commit a596512

Please sign in to comment.