Build: pass PATH environment variable to Docker container
#10133
Conversation
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
Locally it tries to reverted back 🤷
Co-authored-by: Santos Gallegos <[email protected]>
|
We told some people we were deploying this today, but it didn't got reviewed/accepted on time. Please, if you have some time today, review it and let me know if it makes sense to deploy a hotfix. |
| @@ -73,7 +73,7 @@ def __init__( | |||
| bin_path=None, | |||
| record_as_success=False, | |||
| demux=False, | |||
| **kwargs, | |||
| **kwargs, # pylint: disable=unused-argument | |||
There was a problem hiding this comment.
This seems like we have linting configured too strongly, if so much of this PR is linting hints... Maybe that's making the code better, but I'm not convinced this check is useful?
| # Prepend the BIN_PATH if it's defined | ||
| if self.bin_path: | ||
| path = environment.get("PATH") | ||
| bin_path = self._escape_command(self.bin_path) |
There was a problem hiding this comment.
Why are we only escaping this here? If this is user input and security critical, it should be very explicitly mentioned.
There was a problem hiding this comment.
I'm not sure why that _escape_command is there. I copied from our previous code,
readthedocs.org/readthedocs/doc_builder/environments.py
Lines 369 to 371 in 830b899
In theory, it's not input from the user. This should be the venv path, if I understand correctly this code.
There was a problem hiding this comment.
It may be not required, I think 🤷🏼
After a quick grep, it's always self.python_env.venv_bin() or self.venv_bin(). So, we could probably remove it if we want, but I'm not sure.
| 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}" | ||
|
|
There was a problem hiding this comment.
This logic feels really weird to me. We're overwriting then appending the original path if it exists, but the variable names don't make that clear. I think we want something closer to:
| 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}" | |
| if self.bin_path: | |
| original_path = environment.get("PATH") | |
| # SECURITY CRITICAL: Escape user input | |
| escaped_bin_path = self._escape_command(self.bin_path) | |
| environment["PATH"] = f"{escaped_bin_path}:{original_path}" |
There was a problem hiding this comment.
We need the if because original_path could be None.
I'm taking the suggestion of the variable names, tho.
|
I updated the PR with the suggestions provided. Let me know what you think.
|
I hope it doesn't 🙏🏼 (thinking in positive) 😝 |
|
@humitos Users can't say we didn't warn them. This is exactly why it's still in beta :) |
Quick revert of #10133 until we can think more about it.
Quick revert of #10133 until we can think more about it.
This is another attempt to fix this issue. It follows the suggestion posted by @bensze01 in #10103. It looks simple and I think it makes sense to give it a try since it doesn't seem to be harmful. I tested it locally with the branch https://github.com/readthedocs/test-builds/tree/build-commands-multiline and it worked fine. Note that without this PR the branch linked fails. Related #10133 Related #10119 Related #10206 External PRs that should be solved by this one: * pypi/warehouse#12953 * pypi/warehouse#12953 Closes #10103 Closes #10208
* Build: allow multi-line commands on `build.commands` This is another attempt to fix this issue. It follows the suggestion posted by @bensze01 in #10103. It looks simple and I think it makes sense to give it a try since it doesn't seem to be harmful. I tested it locally with the branch https://github.com/readthedocs/test-builds/tree/build-commands-multiline and it worked fine. Note that without this PR the branch linked fails. Related #10133 Related #10119 Related #10206 External PRs that should be solved by this one: * pypi/warehouse#12953 * pypi/warehouse#12953 Closes #10103 Closes #10208 * Build: use `;` to separate PREFIX from COMMAND
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