Build: build.jobs.post_build produces mysterious shell syntax errors

[rptb1]

Details

• Read the Docs project URL: https://readthedocs.org/projects/mmref/
• Build URL (if applicable): https://readthedocs.org/projects/mmref/builds/19671469/ and https://readthedocs.org/projects/mmref/builds/19671433/ and most previous builds
• Read the Docs username (if applicable): https://readthedocs.org/profiles/rptb1/

Expected Result

A post_build script if test -n "A"; then echo "B"; fi should not produce a syntax error.
A post_build script with sh -c "foo" in it should run the shell with the argument.

Actual Result

/bin/sh: 1: Syntax error: "then" unexpected
sh: 0: -c requires an argument

I'm following the manual at "Extend the build process" and trying to insert a simple shell script at the post_build step.

A script that works locally (Ubuntu 22) is somehow mangled and causes syntax errors when inserted into the .readthedocs.yml at the post_build stage. I have tried many ways of writing the script and quoting it in YAML, to try to eliminate YAML as the culprit:

1. Ravenbrook/mps@4146715
2. Ravenbrook/mps@9fbc622
3. Ravenbrook/mps@2a64259
4. Ravenbrook/mps@7fdbbd3
5. Ravenbrook/mps@82f0d18
6. Ravenbrook/mps@044e790
7. Ravenbrook/mps@4a07b84
8. Ravenbrook/mps@1106c9c

The last one is especially mysterious.

Is there some sort of mangling of the script going on, or am I just going blind?

None of these scripts produce syntax errors when I feed them to /bin/sh on my local Ubuntu. I have written similar scripts for multiple CI systems without coming across this problem before.

Thanks.

[rptb1]

A literal copy of your own example produces a syntax error.

I coped the text of the script from https://docs.readthedocs.io/en/stable/build-customization.html#id3 exactly in Ravenbrook/mps@b08bcf3 and it causes the syntax error seen at https://readthedocs.org/projects/mmref/builds/19671564/

[humitos]

Thanks for reporting this issue. We have been dealing with this already at pypi/warehouse#12953 and I wasn't able to find the cause of it at that time. We will need to dig deeper into the code to understand what's going on here.

[rptb1]

We will need to dig deeper into the code to understand what's going on here.

This split looks highly suspicious.

readthedocs.org/readthedocs/doc_builder/director.py

Line 333 in f4215c5

environment.run(*command.split(), escape_command=False, cwd=cwd)

From a 10 minute scan of your code, it looks to me like there might be a confusion between executing a single command and executing a script.

On a single command, where you want to split argv and do something like run(['ls', '-l']), because you're basically doing something like a call to exec(3) and spawing a single process.

But a script is a single string that you want to pass complete to a shell, like run(['/bin/sh', '-c', SCRIPT]) (or similar). Splitting the script makes no sense at all.

>>> post_build = """
... if test -n "${MMREF}"; then \
...          mv "${READTHEDOCS_OUTPUT}/html" "${READTHEDOCS_OUTPUT}/mmref.in" && \
...          python manual/make-mmref.py "${READTHEDOCS_OUTPUT}/mmref.in" "${READTHEDOCS_OUTPUT}/html" \
...     fi
... """
>>> post_build.split()
['if', 'test', '-n', '"${MMREF}";', 'then', 'mv', '"${READTHEDOCS_OUTPUT}/html"', '"${READTHEDOCS_OUTPUT}/mmref.in"', '&&', 'python', 'manual/make-mmref.py', '"${READTHEDOCS_OUTPUT}/mmref.in"', '"${READTHEDOCS_OUTPUT}/html"', 'fi']

Again, I am in no way sure this is what's wrong, but your splits don't look right to me.

[ericholscher]

@rptb1 I'm curious if it works if you put the script in a file, and just run that from the post-build step? Not ideal, but I don't think our indention with this feature was having full-length scripts here, but having commands or scripts that could be run. We can definitely change that design, but curious if it works that way.

That said, if our docs examples aren't working, we should probably fix them :)

[rptb1]

That might be a workaround for now, thanks.

The intention is clear from the documentation: currently the documented examples don't work.

You'll need to carefully document the subset of shell language you do support. Why not just pass everything to the shell? Is there a reason not to?

[humitos]

I opened a PR that removes the .split() on those command and interprets the text as a whole. I did some small QA locally and it worked. I'd appreciate some review on that code since I'm not 100% sure that it's not gonna cause other issues.

[humitos]

I'm re-opening this issue because the PR that was merged doesn't solve all the issues.
I'm still not sure what's the best way to solve this problem 🤷🏼 . This case, for example, is not solved:

That exact same code works properly in my local terminal. Even wrapping it in /bin/sh -c '{cmd}' as we are doing in our code. It seems we are doing some manipulation of the command that breaks it, or, there is a problem when communicating with Docker.

This requires more researching and testing. Suggestions and ideas welcomed!

[humitos]

I think the main problem here is the PATH= environment variable prefix we are adding:

It works without the prefix:

▶ sh -c 'if [ "$READTHEDOCS_VERSION_TYPE" = "external" ] && git diff --quiet origin/main -- docs/ .readthedocs.yaml;
then
 exit 183;
fi'

It fails with the prefix

▶ sh -c 'PATH=/bin:$PATH if [ "$READTHEDOCS_VERSION_TYPE" = "external" ] && git diff --quiet origin/main -- docs/ .readthedocs.yaml;
then
 exit 183;
fi'
sh: line 1: if: command not found
sh: -c: line 2: syntax error near unexpected token `then'
sh: -c: line 2: `then'

This code is at

readthedocs.org/readthedocs/doc_builder/environments.py

Lines 356 to 384 in dd07a3e

	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,
	)
	)

[humitos]

Another simplified example that breaks because of the environment variable prefix:

▶ sh -c 'if [ "1" ]; then echo 1; fi'
1

▶ sh -c 'FOO=bar if [ "1" ]; then echo 1; fi'
sh: -c: line 1: syntax error near unexpected token `then'
sh: -c: line 1: `FOO=bar if [ "1" ]; then echo 1; fi'

[humitos]

I opened 2 PRs to solve this issue:

• Build: pass PATH environment variable to Docker container #10133 moves all the environment variable management to the Docker create_exec call
• asdf: update asdf and all its plugins readthedocs-docker-images#189 updates asdf in our Docker images to auto reshim executables installed via pip

With these two, I think we will be solving most of our users' issues avoiding unexpected behaviors.

[ericholscher]

@humitos Great work tracking this down!

[humitos]

We deployed a fix twice for this but we had other issues, so we had to roll them back. I'm re-opening this issue because the problem is not fixed in production.

[bensze01]

So, I've run into this issue as well (Mbed-TLS/mbedtls#7538) - The workaround I settled on is to insert a newline before the if statement.
However reading through @humitos's explanation, this probably breaks makes the PATH= statement ineffective.
I guess if passing this variable to docker had to be rolled back, you could make the commands look like this instead

export PATH=${foo}
${command_from_yaml}

[bensze01]

Using the simplified example from @humitos:

$ sh -c 'FOO=bar if [ "1" ]; then echo 1; fi;'
sh: -c: line 1: syntax error near unexpected token 'then'
sh: -c: line 1: 'FOO=bar if [ "1" ]; then echo 1; fi;'

$ sh -c 'export FOO=bar
if [ "1" ]; then echo 1; fi;'
1