-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add support for "packages" install key in v2 config #7289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
9d018b0
7e57736
4582e58
2f9c98b
899667e
bf773e8
d906366
c776387
3362072
0a99cbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,11 @@ | |
|
||
from readthedocs.builds.constants import EXTERNAL | ||
from readthedocs.config import PIP, SETUPTOOLS, ParseError, parse as parse_yaml | ||
from readthedocs.config.models import PythonInstall, PythonInstallRequirements | ||
from readthedocs.config.models import ( | ||
PythonInstall, | ||
PythonInstallRequirements, | ||
PythonInstallPackages, | ||
) | ||
from readthedocs.doc_builder.config import load_yaml_config | ||
from readthedocs.doc_builder.constants import DOCKER_IMAGE | ||
from readthedocs.doc_builder.environments import DockerBuildEnvironment | ||
|
@@ -77,6 +81,8 @@ def install_requirements(self): | |
for install in self.config.python.install: | ||
if isinstance(install, PythonInstallRequirements): | ||
self.install_requirements_file(install) | ||
if isinstance(install, PythonInstallPackages): | ||
self.install_packages_list(install) | ||
if isinstance(install, PythonInstall): | ||
self.install_package(install) | ||
|
||
|
@@ -430,6 +436,33 @@ def install_requirements_file(self, install): | |
bin_path=self.venv_bin(), | ||
) | ||
|
||
def install_packages_list(self, install): | ||
""" | ||
Install requirements from a list of strings using pip. | ||
|
||
:param install: A instal object from the config module. | ||
:type install: readthedocs.config.modules.PythonInstallPackages | ||
""" | ||
|
||
args = [ | ||
self.venv_bin(filename='python'), | ||
'-m', | ||
'pip', | ||
'install', | ||
] | ||
if self.project.has_feature(Feature.PIP_ALWAYS_UPGRADE): | ||
args += ['--upgrade'] | ||
args += [ | ||
'--exists-action=w', | ||
*self._pip_cache_cmd_argument(), | ||
] | ||
args += install.packages | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be careful here, users could set a package name like Invalid names would be those that start with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How much of a concern is this really? Pip also reads options from
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, you are right, maybe allow the same subset of options? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about using the same parser that pip uses and check for a valid name? On the other hand, @davidfischer created this package that could be useful if we can validate each line separately: https://requirements-parser.readthedocs.io/en/latest/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does RTD have a separate validation step? Otherwise Pip will just complain if the requirement is invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but we need to verify it before sending to pip in some way because otherwise you can inject random commands into the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This package is super old and outdated and probably would need some love before it's useful. That package doesn't directly parse the lines by itself but instead uses the parser built-in to setuptools. |
||
self.build_env.run( | ||
*args, | ||
cwd=self.checkout_path, | ||
bin_path=self.venv_bin(), | ||
) | ||
|
||
def list_packages_installed(self): | ||
"""List packages installed in pip.""" | ||
args = [ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider supporting private packages https://docs.readthedocs.io/en/stable/guides/private-python-packages.html for Read the Docs for Business, probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I am missing something, this should already be supported implicitly. You can just use a Git repository URL instead of a package name, e.g.:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested that the environment variable is properly replaced with its value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope and I doubt that it will be at this stage. Good point that we should probably support the substitution of environment variables, otherwise while it technically works to hard code the token, that's probably not what anyone wants 😄
Does Pip support the substitution in
requirements.txt
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, pip supports expanding variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'll be easier to just write the arguments into a temporary requirements file and re-use the existing logic for installing from that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to push this along and finish this work, I'd be fine opening an issue to add private repository support to the
packages
config key later. This PR seems close otherwise and it looks to be blocked on this additional feature.