Open XuehaiPan opened 2 years ago
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 😞
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?
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
.
@humitos
The BuildEnvironment
accepts both str
and List[str]
as command:
It seems unnecessary to word splitting.
We split the command in
Then join them again before executing:
EDIT to https://github.com/readthedocs/readthedocs.org/issues/9397#issuecomment-1173809071:
Only run BuildEnvironment
with argument shell=True
will join the command parts into a single str
:
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.
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).
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.
This is related to #10103 and it may be fixed by the PR associated as well.
This error happens because we wrap the command as
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.
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
?
@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.
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 {} \;
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.
@ThiefMaster you can put your command/script into a file and run it from Read the Docs as mentioned in https://github.com/readthedocs/readthedocs.org/issues/9397#issuecomment-1173795211 as a workaround.
would be overkill for this, but good to know it's possible
https://github.com/indico/indico/commit/420ddfb8d42f142f67ca94957ee3a87e0df70e21
pre_install:
# RTD python versions are usually behind a bit...
- sed -i -E "/^python_requires = /d" setup.cfg
Details
Expected Result
A description of what you wanted to happen
Run
build.jobs
command successfullyMy
.readthedocs.yaml
:I want to run command:
after installing the dependencies.
Actual Result
A description of what actually happened
Got errors in raw output:
In:
https://github.com/readthedocs/readthedocs.org/blob/62effc771a6171f9252dd8bbaf779cf8c7d0a975/readthedocs/doc_builder/director.py#L339-L341
we split the command with whitespace into a list of command-line options. In my use case, I have:
Expected split: