Fail to run build.jobs with single quotes (') in commands

[XuehaiPan]

Details

• Read the Docs project URL: https://readthedocs.org/projects/nvitop/
• Build URL (if applicable): https://readthedocs.org/projects/nvitop/builds/17338573/
• Read the Docs username (if applicable): https://readthedocs.org/profiles/XuehaiPan/

Expected Result

A description of what you wanted to happen

Run build.jobs command successfully

My .readthedocs.yaml:

# .readthedocs.yaml
# Read the Docs configuration file
# See https://docs.readthedocs.io/en/stable/config-file/v2.html for details

# Required
version: 2

# Set the version of Python and other tools you might need
build:
  os: ubuntu-22.04
  tools:
    python: "3.8"
  jobs:
    post_install:
      - sed -i -E 's/^     process identity for every yielded instance$/  \0/' "$(python3 -c "print(__import__('psutil').__file__)")"

# Build documentation in the docs/ directory with Sphinx
sphinx:
  builder: html
  configuration: docs/source/conf.py
  fail_on_warning: true

# Optionally declare the Python requirements required to build your docs
python:
  install:
    - requirements: requirements.txt
    - requirements: docs/requirements.txt
  system_packages: true

I want to run command:

sed -i -E 's/^     process identity for every yielded instance$/  \0/' "$(python3 -c "print(__import__('psutil').__file__)")"

after installing the dependencies.

Actual Result

A description of what actually happened

Got errors in raw output:

[rtd-command-info] start-time: 2022-07-02T10:39:42.282806Z, end-time: 2022-07-02T10:39:42.362971Z, duration: 0, exit-code: 1
sed -i -E 's/^ process identity for every yielded instance$/ \0/' "$(python3 -c "print(__import__('psutil').__file__)")"
sed: -e expression #1, char 3: unterminated `s' command

In:

readthedocs.org/readthedocs/doc_builder/director.py

Lines 339 to 341 in 62effc7

	commands = getattr(self.data.config.build.jobs, job, [])
	for command in commands:
	environment.run(*command.split(), escape_command=False, cwd=cwd)

we split the command with whitespace into a list of command-line options. In my use case, I have:

>>> command = r'''sed -i -E 's/^     process identity for every yielded instance$/  \0/' "$(python3 -c "print(__import__('psutil').__file__)")"'''
>>> print(command)
sed -i -E 's/^     process identity for every yielded instance$/  \0/' "$(python3 -c "print(__import__('psutil').__file__)")"

>>> command.split()  # split with whitespace
[
    'sed',
    '-i',
    '-E',
    "'s/^",
    'process',
    'identity',
    'for',
    'every',
    'yielded',
    'instance$/',
    "\\0/'",
    '"$(python3',
    '-c',
    '"print(__import__(\'psutil\').__file__)")"'
]

Expected split:

[
    'sed',
    '-i',
    '-E',
    "'s/^     process identity for every yielded instance$/  \\0/'",
    '"$(python3 -c "print(__import__(\'psutil\').__file__)")"'
]

[humitos]

Hi @XuehaiPan! By using double quotes (") instead of single quotes (') you can work around this issue.

sed -i -E "s/^     process identity for every yielded instance$/  \0/" "$(python3 -c "print(__import__('psutil').__file__)")"

I did a quick test in https://readthedocs.org/projects/test-builds/builds/17348985/ and it worked. It passed the build, but it removed the initial spaces from the sed replacement string 😞

[humitos]

We could use shlex.split instead of regular split. It splits the command as we want, but it removes the ' so the command fails anyways:

In [1]: import shlex

In [2]: command = r'''sed -i -E 's/^     process identity for every yielded instance$/  \0/' "$(python3 -c "print(__import__('p
   ...: sutil').__file__)")"'''

In [3]: shlex.split(command)
Out[3]: 
['sed',
 '-i',
 '-E',
 's/^     process identity for every yielded instance$/  \\0/',
 '$(python3 -c print(__import__(psutil).__file__))']

Even with double quotes ("):

In [4]: command = r'''sed -i -E "s/^     process identity for every yielded instance$/  \0/" "$(python3 -c "print(__import__('p
   ...: sutil').__file__)")"'''

In [5]: shlex.split(command)
Out[5]: 
['sed',
 '-i',
 '-E',
 's/^     process identity for every yielded instance$/  \\0/',
 '$(python3 -c print(__import__(psutil).__file__))']

That said, I'm not convinced that shlex.split is better than regular split since escaping the quotes also breaks. I'm curious how other CIs like GitHub Actions or CircleCI handle this. Do you know?

[XuehaiPan]

As a workaround, I created a single Bash script fix-psutil-docstring.sh:

#!/usr/bin/env bash

# shellcheck disable=SC2312
exec sed -i -E 's/^     process identity for every yielded instance$/  \0/' "$(python3 -c "print(__import__('psutil').__file__)")"

Then put:

build:
  os: ubuntu-22.04
  tools:
    python: "3.8"
  jobs:
    post_install:
      - bash docs/source/fix-psutil-docstring.sh

in .readthedocs.yaml.

[XuehaiPan]

@humitos

The BuildEnvironment accepts both str and List[str] as command:

readthedocs.org/readthedocs/doc_builder/environments.py

Lines 39 to 62 in 6bf0bed

	class BuildCommand(BuildCommandResultMixin):
	"""
	Wrap command execution for execution in build environments.
	This wraps subprocess commands with some logic to handle exceptions,
	logging, and setting up the env for the build command.
	This acts a mapping of sorts to the API representation of the
	:py:class:`readthedocs.builds.models.BuildCommandResult` model.
	:param command: string or array of command parameters
	:param cwd: Absolute path used as the current working path for the command.
	Defaults to ``RTD_DOCKER_WORKDIR``.
	:param shell: execute command in shell, default=False
	:param environment: environment variables to add to environment
	:type environment: dict
	:param str user: User used to execute the command, it can be in form of ``user:group``
	or ``user``. Defaults to ``RTD_DOCKER_USER``.
	:param build_env: build environment to use to execute commands
	:param bin_path: binary path to add to PATH resolution
	:param demux: Return stdout and stderr separately.
	:param kwargs: allow to subclass this class and extend it
	"""

It seems unnecessary to word splitting.

---

We split the command in

readthedocs.org/readthedocs/doc_builder/director.py

Lines 334 to 336 in 6bf0bed

	commands = getattr(self.data.config.build.jobs, job, [])
	for command in commands:
	environment.run(*command.split(), escape_command=False, cwd=cwd)

Then join them again before executing:

readthedocs.org/readthedocs/doc_builder/environments.py

Lines 218 to 222 in 6bf0bed

	def get_command(self):
	"""Flatten command."""
	if hasattr(self.command, '__iter__') and not isinstance(self.command, str):
	return ' '.join(self.command)
	return self.command

[XuehaiPan]

EDIT to #9397 (comment):

Only run BuildEnvironment with argument shell=True will join the command parts into a single str:

readthedocs.org/readthedocs/doc_builder/environments.py

Lines 148 to 151 in 6bf0bed

	# When using ``shell=True`` the command should be flatten
	command = self.command
	if self.shell:
	command = self.get_command()

We can change:

     commands = getattr(self.data.config.build.jobs, job, [])
     for command in commands:
-         environment.run(*command.split(), escape_command=False, cwd=cwd)
+         environment.run(['bash', '-c', command], escape_command=False, cwd=cwd) 

Note that environment.run(..., shell=True) won't work because it will remove continuous whitespaces.

[humitos]

I'm not 100% sure about the potential issues of using shell=True here. @agjohnson @ericholscher are you aware of them?

@ericholscher I saw you put this in my sprint, but I think I marked it as low priority since it involves some research about potential security issues by breaking out the Docker container and also because there is a workaround already (put the command inside a .sh file).

[ericholscher]

I thought it was a somewhat simple bug fix when I saw it, related to the builders. I definitely don't want to run things with shell=True, if that's what is required.

[humitos]

This is related to #10103 and it may be fixed by the PR associated as well.

[humitos]

This error happens because we wrap the command as

f"/bin/bash -c '{command}'"

readthedocs.org/readthedocs/doc_builder/environments.py

Lines 386 to 401 in bc4bced

	"""
	Wrap command in a shell and optionally escape special bash characters.
	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.
	"""
	command = (
	' '.join(
	self._escape_command(part) if self.escape_command else part
	for part in self.command
	)
	)
	return f"/bin/bash -c '{command}'"

So, using ' inside user's commands is tricky. Knowing this, you should be able to re-write your command to avoid using ' or to escape them.

[ericholscher]

So, using ' inside user's commands is tricky. Knowing this, you should be able to re-write your command to avoid using ' or to escape them.

This is not a great solution for the user. We should find a better way to do this. Seems like there should be a proper way to handle this? Worst case, we can write the command to a file and run it /bin/bash script.sh?

[humitos]

@ericholscher yeah, I know it's not the best solution 😄 . I commented here since I found the exact reason of the problem now. From there, we can figure it out how to solve it and how to provide better workarounds.

[LecrisUT]

This error happens because we wrap the command as

f"/bin/bash -c '{command}'"

Should be documented with a .. warning

Here's the gem I had to write to circumvent it:

    - find $READTHEDOCS_OUTPUT/html -type f -iname "*.html" -exec sh -c "echo ln -s \$(basename \"\$1\") \$(dirname \"\$1\")/\$(basename \"\$1\" .html)" sh {} \;

[ThiefMaster]

It'd be great if this could be fixed. Depending on the command you run, having to rely only on double quotes makes things much uglier.

[humitos]

@ThiefMaster you can put your command/script into a file and run it from Read the Docs as mentioned in #9397 (comment) as a workaround.

[ThiefMaster]

would be overkill for this, but good to know it's possible

indico/indico@420ddfb

    pre_install:
      # RTD python versions are usually behind a bit...
      - sed -i -E "/^python_requires = /d" setup.cfg