Skip to content

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

Closed
wants to merge 10 commits into from
30 changes: 30 additions & 0 deletions docs/config-file/v2.rst
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,36 @@ With the previous settings, Read the Docs will execute the next commands:
pip install .[docs]
python package/setup.py install

Extra dependencies
''''''''''''''''''

Install packages from a list of requirements.
This allows you to declare dependencies in the ``.readthedocs.yml`` file instead of creating a separate file.

:Key: `packages`
:Type: ``list``
:Default: ``[]``
:Required: ``true``

Example:

.. code-block:: yaml

version: 2

python:
version: 3.7
install:
- packages:
- mkdocs-material
- Pygments
Comment on lines +251 to +252
Copy link
Member

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.

Copy link
Author

@NiklasRosenstein NiklasRosenstein Sep 4, 2020

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.:

- packages:
  - mkdocs-material
  - git+https://${GITHUB_TOKEN}@github.com/user/project.git@{version}

Copy link
Member

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?

Copy link
Author

@NiklasRosenstein NiklasRosenstein Sep 8, 2020

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Pip support the substitution in requirements.txt?

Yes, pip supports expanding variables

Copy link
Author

@NiklasRosenstein NiklasRosenstein Sep 9, 2020

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

Copy link
Contributor

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.


With the previous settings, Read the Docs will execute the next commands:

.. prompt:: bash $

pip install mkdocs-material Pygments

python.system_packages
``````````````````````

Expand Down
13 changes: 13 additions & 0 deletions readthedocs/config/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
Python,
PythonInstall,
PythonInstallRequirements,
PythonInstallPackages,
Search,
Sphinx,
Submodules,
Expand Down Expand Up @@ -833,6 +834,16 @@ def validate_python_install(self, index):
self.base_path,
)
python_install['requirements'] = requirements
elif 'packages' in raw_install:
packages_key = key + '.packages'
packages = self.pop_config(packages_key)
if not isinstance(packages, list):
self.error(
packages_key,
'"{}" key must be a list'.format(packages_key),
code=PYTHON_INVALID,
)
python_install['packages'] = packages
elif 'path' in raw_install:
path_key = key + '.path'
with self.catch_validation_error(path_key):
Expand Down Expand Up @@ -1112,6 +1123,8 @@ def python(self):
for install in python['install']:
if 'requirements' in install:
python_install.append(PythonInstallRequirements(**install),)
elif 'packages' in install:
python_install.append(PythonInstallPackages(**install),)
elif 'path' in install:
python_install.append(PythonInstall(**install),)
return Python(
Expand Down
5 changes: 5 additions & 0 deletions readthedocs/config/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ class PythonInstallRequirements(Base):
__slots__ = ('requirements',)


class PythonInstallPackages(Base):

__slots__ = ('packages',)


class PythonInstall(Base):

__slots__ = (
Expand Down
21 changes: 21 additions & 0 deletions readthedocs/config/tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
Conda,
PythonInstall,
PythonInstallRequirements,
PythonInstallPackages,
)
from readthedocs.config.validation import (
INVALID_BOOL,
Expand Down Expand Up @@ -1104,6 +1105,26 @@ def test_python_install_requirements_check_valid(self, tmpdir):
assert isinstance(install[0], PythonInstallRequirements)
assert install[0].requirements == 'requirements.txt'

def test_python_install_packages_check_valid(self, tmpdir):
build = self.get_build_config(
{
'python': {
'install': [{
'packages': [
'package_a',
'package_b >=3.0.0,<4.0.0',
],
}],
},
},
source_file=str(tmpdir.join('readthedocs.yml')),
)
build.validate()
install = build.python.install
assert len(install) == 1
assert isinstance(install[0], PythonInstallPackages)
assert install[0].packages == ['package_a', 'package_b >=3.0.0,<4.0.0']

def test_python_install_requirements_does_not_allow_null(self, tmpdir):
build = self.get_build_config(
{
Expand Down
35 changes: 34 additions & 1 deletion readthedocs/doc_builder/python_environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be careful here, users could set a package name like --option and we will execute that. Not sure the best way to prevent that. We could probably validate this in the config module and raise an exception for invalid names, or maybe escape those characters here. /cc @readthedocs/core

Invalid names would be those that start with -, not sure what else we should validate here.

Copy link
Author

Choose a reason for hiding this comment

The 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 requirements.txt files, or at least a subset of options that are valid for the pip install command like

--extra-index-url https://example.org/pypi/simple
some-requirement >=1.0.0, <2.0.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, you are right, maybe allow the same subset of options?

Copy link
Member

Choose a reason for hiding this comment

The 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/

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 pip by calling pip install package1 "echo test > /tmp/test.txt" (quotes should be `, but I can escape them here)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

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 = [
Expand Down