From 09c0c87d7203ed8a7d30b393613e023e1e5d9017 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 8 Oct 2018 15:59:06 -0500 Subject: [PATCH 01/28] Extend validation for config.install --- readthedocs/config/config.py | 97 ++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 31 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 9d2217bd257..628bdf81401 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -40,6 +40,8 @@ ) ALL = 'all' +PIP = 'pip' +SETUPTOOLS = 'setuptools' CONFIG_FILENAME_REGEX = r'^\.?readthedocs.ya?ml$' CONFIG_NOT_SUPPORTED = 'config-not-supported' @@ -602,7 +604,7 @@ class BuildConfigV2(BuildConfigBase): valid_formats = ['htmlzip', 'pdf', 'epub'] valid_build_images = ['1.0', '2.0', '3.0', 'stable', 'latest'] default_build_image = 'latest' - valid_install_options = ['pip', 'setup.py'] + valid_install_method = [PIP, SETUPTOOLS] valid_sphinx_builders = { 'html': 'sphinx', 'htmldir': 'sphinx_htmldir', @@ -725,38 +727,21 @@ def validate_python(self): self.get_valid_python_versions(), ) - with self.catch_validation_error('python.requirements'): - requirements = self.defaults.get('requirements_file') - requirements = self.pop_config('python.requirements', requirements) - if requirements != '' and requirements is not None: - requirements = validate_file(requirements, self.base_path) - python['requirements'] = requirements - with self.catch_validation_error('python.install'): - install = ( - 'setup.py' if self.defaults.get('install_project') else None - ) - install = self.pop_config('python.install', install) - if install is not None: - validate_choice(install, self.valid_install_options) - python['install_with_setup'] = install == 'setup.py' - python['install_with_pip'] = install == 'pip' - - with self.catch_validation_error('python.extra_requirements'): - extra_requirements = self.pop_config( - 'python.extra_requirements', [] - ) - extra_requirements = validate_list(extra_requirements) - if extra_requirements and not python['install_with_pip']: - self.error( - 'python.extra_requirements', - 'You need to install your project with pip ' - 'to use extra_requirements', - code=PYTHON_INVALID, + raw_install = self.raw_config.get('python', {}).get('install', []) + validate_list(raw_install) + if raw_install: + self.raw_config.setdefault('python', {})['install'] = ( + self._list_to_dict(raw_install) ) - python['extra_requirements'] = [ - validate_string(extra) for extra in extra_requirements - ] + else: + self.pop_config('python.install') + + raw_install = self.raw_config.get('python', {}).get('install', []) + python['install'] = [ + self.validate_python_install(index) + for index in range(len(raw_install)) + ] with self.catch_validation_error('python.system_packages'): system_packages = self.defaults.get( @@ -771,6 +756,56 @@ def validate_python(self): return python + def _list_to_dict(self, list_): + dict_ = { + str(i): element + for i, element in enumerate(list_) + } + return dict_ + + def validate_python_install(self, index): + python_install = {} + key = 'python.install.{}'.format(index) + if 'requirements' in self.raw_config.get(key): + requirements_key = key + '.requirements' + with self.catch_validation_error(requirements_key): + requirements = validate_file( + self.pop_config(requirements_key), + self.base_path + ) + python_install['requirements'] = requirements + elif 'path' in self.raw_config.get(key): + path_key = key + '.path' + with self.catch_validation_error(path_key): + path = validate_directory( + self.pop_config(path_key), + self.base_path + ) + python_install['path'] = path + + method_key = key + '.method' + with self.catch_validation_error(method_key): + method = validate_choice( + self.pop_config(method_key, PIP), + self.valid_install_method + ) + python_install['method'] = method + + extrareq_key = key + '.extra_requirements' + with self.catch_validation_error(extrareq_key): + extra_requirements = validate_list( + self.pop_config(extrareq_key, []) + ) + if extra_requirements and python_install['method'] != PIP: + self.error( + 'python.install.extra_requirements', + 'You need to install your project with pip ' + 'to use extra_requirements', + code=PYTHON_INVALID, + ) + python_install['extra_requirements'] = extra_requirements + return python_install + def get_valid_python_versions(self): """ Get the valid python versions for the current docker image. From c817d4b3dadf465d4068a27631be979cfbec3fd3 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 8 Oct 2018 16:49:23 -0500 Subject: [PATCH 02/28] Use new models --- readthedocs/config/config.py | 53 ++++++++++++++++++++++++++++++++++-- readthedocs/config/models.py | 19 +++++++++++-- 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 628bdf81401..55157b4b82c 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -14,7 +14,16 @@ from readthedocs.projects.constants import DOCUMENTATION_CHOICES from .find import find_one -from .models import Build, Conda, Mkdocs, Python, Sphinx, Submodules +from .models import ( + Build, + Conda, + Mkdocs, + Python, + PythonInstall, + PythonInstallRequirements, + Sphinx, + Submodules, +) from .parser import ParseError, parse from .validation import ( VALUE_NOT_FOUND, @@ -550,9 +559,36 @@ def formats(self): @property def python(self): """Python related configuration.""" + python = self._config['python'] requirements = self._config['requirements_file'] - self._config['python']['requirements'] = requirements - return Python(**self._config['python']) + python_install = [] + if requirements is not None: + python_install.append( + PythonInstallRequirements( + requirements=requirements, + ) + ) + if python['install_with_pip']: + python_install.append( + PythonInstall( + path=self.base_path, + method=PIP, + extra_requirements=python['extra_requirements'], + ) + ) + if python['install_with_setup']: + python_install.append( + PythonInstall( + path=self.base_path, + method=SETUPTOOLS, + ) + ) + + return Python( + version=python['version'], + install=python_install, + use_system_site_packages=python['use_system_site_packages'], + ) @property def conda(self): @@ -1042,6 +1078,17 @@ def build(self): @property def python(self): + python_install = [] + for install in self._config['python']['install']: + if 'requirements' in install: + python_install.append( + PythonInstallRequirements(**install) + ) + elif 'path' in install: + python_install.append( + PythonInstall(**install) + ) + self._config['python']['install'] = python_install return Python(**self._config['python']) @property diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index bf12ddfa6d4..5efe590a189 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -11,11 +11,24 @@ 'Python', [ 'version', + 'install', + 'use_system_site_packages', + ], +) + +PythonInstallRequirements = namedtuple( + 'PythonInstallRequirements', + [ 'requirements', - 'install_with_pip', - 'install_with_setup', + ], +) + +PythonInstall = namedtuple( + 'PythonInstall', + [ + 'path', + 'method', 'extra_requirements', - 'use_system_site_packages', ], ) From dae2c8b4862d71ec0663d8402bd2929fd6fa8b47 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 8 Oct 2018 17:25:33 -0500 Subject: [PATCH 03/28] Fix tests for v1 --- readthedocs/config/tests/test_config.py | 33 ++++++++++++++----------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 4ba0b5c0982..fe51f238a2d 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -30,7 +30,7 @@ PYTHON_INVALID, VERSION_INVALID, ) -from readthedocs.config.models import Conda +from readthedocs.config.models import Conda, PythonInstall, PythonInstallRequirements from readthedocs.config.validation import ( INVALID_BOOL, INVALID_CHOICE, @@ -293,16 +293,17 @@ def test_python_pip_install_default(): build = get_build_config({'python': {}}, get_env_config()) build.validate() # Default is False. - assert build.python.install_with_pip is False + install = build.python.install + assert len(install) == 0 def describe_validate_python_extra_requirements(): - def it_defaults_to_list(): + def it_defaults_to_not_install(): build = get_build_config({'python': {}}, get_env_config()) build.validate() - # Default is an empty list. - assert build.python.extra_requirements == [] + install = build.python.install + assert len(install) == 0 def it_validates_is_a_list(): build = get_build_config( @@ -363,7 +364,8 @@ def describe_validate_setup_py_install(): def it_defaults_to_false(): build = get_build_config({'python': {}}, get_env_config()) build.validate() - assert build.python.install_with_setup is False + install = build.python.install + assert len(install) == 0 def it_validates_value(): build = get_build_config( @@ -560,9 +562,7 @@ def test_valid_build_config(): assert build.name == 'docs' assert build.base assert build.python - assert build.python.install_with_setup is False - assert build.python.install_with_pip is False - assert build.python.use_system_site_packages is False + assert len(build.python.install) == 0 assert build.output_base @@ -733,7 +733,7 @@ def test_validates_conda_file(tmpdir): def test_requirements_file_empty(): build = get_build_config({}, get_env_config()) build.validate() - assert build.python.requirements is None + assert len(build.python.install) == 0 def test_requirements_file_repects_default_value(tmpdir): @@ -747,7 +747,9 @@ def test_requirements_file_repects_default_value(tmpdir): source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.requirements == 'myrequirements.txt' + install = build.python.install + assert len(install) == 1 + assert install[0].requirements == 'myrequirements.txt' def test_requirements_file_respects_configuration(tmpdir): @@ -758,7 +760,9 @@ def test_requirements_file_respects_configuration(tmpdir): source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.requirements == 'requirements.txt' + install = build.python.install + assert len(install) == 1 + assert install[0].requirements == 'requirements.txt' def test_requirements_file_is_null(tmpdir): @@ -768,7 +772,7 @@ def test_requirements_file_is_null(tmpdir): source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.requirements is None + assert len(build.python.install) == 0 def test_requirements_file_is_blank(tmpdir): @@ -778,7 +782,8 @@ def test_requirements_file_is_blank(tmpdir): source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.requirements is None + install = build.python.install + assert len(install) == 0 def test_build_validate_calls_all_subvalidators(tmpdir): From fb25527e524cb6b530bcf63c17d44e4aa163cc65 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 8 Oct 2018 23:08:06 -0500 Subject: [PATCH 04/28] More checks --- readthedocs/config/config.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 55157b4b82c..374b94a30d0 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -45,7 +45,9 @@ 'ConfigError', 'ConfigOptionNotSupportedError', 'InvalidConfig', + 'PIP', 'ProjectConfig', + 'SETUPTOOLS', ) ALL = 'all' @@ -802,7 +804,11 @@ def _list_to_dict(self, list_): def validate_python_install(self, index): python_install = {} key = 'python.install.{}'.format(index) - if 'requirements' in self.raw_config.get(key): + raw_install = self.raw_config['python']['install'][str(index)] + with self.catch_validation_error(key): + validate_dict(raw_install) + + if 'requirements' in raw_install: requirements_key = key + '.requirements' with self.catch_validation_error(requirements_key): requirements = validate_file( @@ -810,7 +816,7 @@ def validate_python_install(self, index): self.base_path ) python_install['requirements'] = requirements - elif 'path' in self.raw_config.get(key): + elif 'path' in raw_install: path_key = key + '.path' with self.catch_validation_error(path_key): path = validate_directory( @@ -834,7 +840,7 @@ def validate_python_install(self, index): ) if extra_requirements and python_install['method'] != PIP: self.error( - 'python.install.extra_requirements', + extrareq_key, 'You need to install your project with pip ' 'to use extra_requirements', code=PYTHON_INVALID, From f98103f605b44df3802de17ed2e2e87b65eaf097 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 8 Oct 2018 23:22:18 -0500 Subject: [PATCH 05/28] Fix tests for v2 --- readthedocs/config/tests/test_config.py | 336 +++++++++++++++++------- 1 file changed, 234 insertions(+), 102 deletions(-) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index fe51f238a2d..378b1bf7cb3 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -18,6 +18,8 @@ ConfigOptionNotSupportedError, InvalidConfig, ProjectConfig, + PIP, + SETUPTOOLS, load, ) from readthedocs.config.config import ( @@ -1091,41 +1093,106 @@ def test_python_version_check_invalid_types(self, value): build.validate() assert excinfo.value.key == 'python.version' - def test_python_requirements_check_valid(self, tmpdir): + def test_python_install_check_default(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) + build.validate() + install = build.python.install + assert len(install) == 1 + assert isinstance(install[0], PythonInstall) + assert install[0].path == str(tmpdir) + assert install[0].method == PIP + assert install[0].extra_requirements == [] + + @pytest.mark.parametrize('value', ['invalid', 'apt']) + def test_python_install_check_invalid(self, value): + build = self.get_build_config({'python': {'install': value}},) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == 'python.install' + + def test_python_install_requirements_check_valid(self, tmpdir): apply_fs(tmpdir, {'requirements.txt': ''}) build = self.get_build_config( - {'python': {'requirements': 'requirements.txt'}}, + { + 'python': { + 'install': [{ + 'requirements': 'requirements.txt' + }], + }, + }, source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.requirements == str(tmpdir.join('requirements.txt')) + install = build.python.install + assert len(install) == 1 + assert isinstance(install[0], PythonInstallRequirements) + assert install[0].requirements == str(tmpdir.join('requirements.txt')) - def test_python_requirements_check_invalid(self, tmpdir): + def test_python_install_requirements_check_invalid(self, tmpdir): apply_fs(tmpdir, {'requirements.txt': ''}) build = self.get_build_config( - {'python': {'requirements': 'invalid'}}, + { + 'python': { + 'install': [{ + 'path': '.', + 'requirements': 'invalid', + }], + }, + }, source_file=str(tmpdir.join('readthedocs.yml')), ) with raises(InvalidConfig) as excinfo: build.validate() - assert excinfo.value.key == 'python.requirements' + assert excinfo.value.key == 'python.install.0.requirements' - def test_python_requirements_default_value(self): + def test_python_install_default_value(self): build = self.get_build_config({}) build.validate() - assert build.python.requirements is None + install = build.python.install + assert len(install) == 0 - def test_python_requirements_allow_null(self): - build = self.get_build_config({'python': {'requirements': None}},) - build.validate() - assert build.python.requirements is None + def test_python_install_requirements_does_not_allow_null(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'requirements': None, + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == 'python.install.0.requirements' - def test_python_requirements_allow_empty_string(self): - build = self.get_build_config({'python': {'requirements': ''}},) - build.validate() - assert build.python.requirements == '' + def test_python_install_requirements_does_not_allow_empty_string(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'requirements': '', + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == 'python.install.0.requirements' - def test_python_requirements_respects_default(self, tmpdir): + def test_python_install_requirements_ignores_default(self, tmpdir): apply_fs(tmpdir, {'requirements.txt': ''}) build = self.get_build_config( {}, @@ -1133,147 +1200,212 @@ def test_python_requirements_respects_default(self, tmpdir): source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.requirements == str(tmpdir.join('requirements.txt')) + assert build.python.install == [] - def test_python_requirements_priority_over_default(self, tmpdir): + def test_python_install_requirements_priority_over_default(self, tmpdir): apply_fs(tmpdir, {'requirements.txt': ''}) build = self.get_build_config( - {'python': {'requirements': 'requirements.txt'}}, + { + 'python': { + 'install': [{ + 'requirements': 'requirements.txt' + }], + }, + }, {'defaults': {'requirements_file': 'requirements-default.txt'}}, source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.requirements == str(tmpdir.join('requirements.txt')) + install = build.python.install + assert len(install) == 1 + assert install[0].requirements == str(tmpdir.join('requirements.txt')) @pytest.mark.parametrize('value', [3, [], {}]) - def test_python_requirements_check_invalid_types(self, value): - build = self.get_build_config({'python': {'requirements': value}},) + def test_python_install_requirements_check_invalid_types(self, value, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'requirements': value, + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) with raises(InvalidConfig) as excinfo: build.validate() - assert excinfo.value.key == 'python.requirements' + assert excinfo.value.key == 'python.install.0.requirements' - def test_python_install_pip_check_valid(self): - build = self.get_build_config({'python': {'install': 'pip'}},) + def test_python_install_pip_check_valid(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'method': 'pip', + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) build.validate() - assert build.python.install_with_pip is True - assert build.python.install_with_setup is False + install = build.python.install + assert len(install) == 1 + assert install[0].path == str(tmpdir) + assert install[0].method == PIP - def test_python_install_pip_priority_over_default(self): + def test_python_install_pip_have_priority_over_default(self, tmpdir): build = self.get_build_config( - {'python': {'install': 'pip'}}, + { + 'python': { + 'install': [{ + 'path': '.', + 'method': 'pip', + }], + }, + }, {'defaults': {'install_project': True}}, + source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.install_with_pip is True - assert build.python.install_with_setup is False + install = build.python.install + assert len(install) == 1 + assert install[0].path == str(tmpdir) + assert install[0].method == PIP - def test_python_install_setuppy_check_valid(self): - build = self.get_build_config({'python': {'install': 'setup.py'}},) + def test_python_install_setuptools_check_valid(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'method': 'setuptools', + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) build.validate() - assert build.python.install_with_setup is True - assert build.python.install_with_pip is False + install = build.python.install + assert len(install) == 1 + assert install[0].path == str(tmpdir) + assert install[0].method == SETUPTOOLS - def test_python_install_setuppy_respects_default(self): + def test_python_install_setuptools_ignores_default(self): build = self.get_build_config( {}, {'defaults': {'install_project': True}}, ) build.validate() - assert build.python.install_with_pip is False - assert build.python.install_with_setup is True + assert build.python.install == [] - def test_python_install_setuppy_priority_over_default(self): + def test_python_install_setuptools_priority_over_default(self, tmpdir): build = self.get_build_config( - {'python': {'install': 'setup.py'}}, + { + 'python': { + 'install': [{ + 'path': '.', + 'method': 'setuptools', + }], + }, + }, {'defaults': {'install_project': False}}, + source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert build.python.install_with_pip is False - assert build.python.install_with_setup is True - - @pytest.mark.parametrize('value', ['invalid', 'apt']) - def test_python_install_check_invalid(self, value): - build = self.get_build_config({'python': {'install': value}},) - with raises(InvalidConfig) as excinfo: - build.validate() - assert excinfo.value.key == 'python.install' + install = build.python.install + assert len(install) == 1 + assert install[0].path == str(tmpdir) + assert install[0].method == SETUPTOOLS - def test_python_install_allow_null(self): - build = self.get_build_config({'python': {'install': None}},) + def test_python_install_allow_empty_list(self): + build = self.get_build_config({'python': {'install': []}},) build.validate() - assert build.python.install_with_pip is False - assert build.python.install_with_setup is False + assert build.python.install == [] def test_python_install_default(self): build = self.get_build_config({'python': {}}) build.validate() - assert build.python.install_with_pip is False - assert build.python.install_with_setup is False + assert build.python.install == [] - @pytest.mark.parametrize('value', [2, [], {}]) + @pytest.mark.parametrize('value', [2, 'string', {}]) def test_python_install_check_invalid_type(self, value): build = self.get_build_config({'python': {'install': value}},) with raises(InvalidConfig) as excinfo: build.validate() assert excinfo.value.key == 'python.install' - def test_python_extra_requirements_and_pip(self): - build = self.get_build_config({ - 'python': { - 'install': 'pip', - 'extra_requirements': ['docs', 'tests'], - } - }) + def test_python_install_extra_requirements_and_pip(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'method': 'pip', + 'extra_requirements': ['docs', 'tests'], + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) build.validate() - assert build.python.extra_requirements == ['docs', 'tests'] - - def test_python_extra_requirements_not_install(self): - build = self.get_build_config({ - 'python': { - 'extra_requirements': ['docs', 'tests'], - } - }) - with raises(InvalidConfig) as excinfo: - build.validate() - assert excinfo.value.key == 'python.extra_requirements' + install = build.python.install + assert len(install) == 1 + assert install[0].extra_requirements == ['docs', 'tests'] - def test_python_extra_requirements_and_setup(self): - build = self.get_build_config({ - 'python': { - 'install': 'setup.py', - 'extra_requirements': ['docs', 'tests'], - } - }) + def test_python_install_extra_requirements_and_setuptools(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'method': 'setuptools', + 'extra_requirements': ['docs', 'tests'], + }], + } + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) with raises(InvalidConfig) as excinfo: build.validate() - assert excinfo.value.key == 'python.extra_requirements' + assert excinfo.value.key == 'python.install.0.extra_requirements' @pytest.mark.parametrize('value', [2, 'invalid', {}]) - def test_python_extra_requirements_check_type(self, value): - build = self.get_build_config({ - 'python': { - 'install': 'pip', - 'extra_requirements': value, + def test_python_install_extra_requirements_check_type(self, value, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'method': 'pip', + 'extra_requirements': value, + }], + }, }, - }) + source_file=str(tmpdir.join('readthedocs.yml')), + ) with raises(InvalidConfig) as excinfo: build.validate() - assert excinfo.value.key == 'python.extra_requirements' + assert excinfo.value.key == 'python.install.0.extra_requirements' - def test_python_extra_requirements_allow_empty(self): - build = self.get_build_config({ - 'python': { - 'install': 'pip', - 'extra_requirements': [], + def test_python_install_extra_requirements_allow_empty(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '', + 'method': 'pip', + 'extra_requirements': [], + }], + }, }, - }) - build.validate() - assert build.python.extra_requirements == [] - - def test_python_extra_requirements_check_default(self): - build = self.get_build_config({}) + source_file=str(tmpdir.join('readthedocs.yml')), + ) build.validate() - assert build.python.extra_requirements == [] + install = build.python.install + assert len(install) == 1 + assert install[0].extra_requirements == [] @pytest.mark.parametrize('value', [True, False]) def test_python_system_packages_check_valid(self, value): From 258dff3799a5b552286bf05ed9a703efbe8a02cc Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Oct 2018 11:04:24 -0500 Subject: [PATCH 06/28] More tests --- readthedocs/config/config.py | 6 ++ readthedocs/config/tests/test_config.py | 90 +++++++++++++++++++++++-- 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 374b94a30d0..07610faafcb 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -846,6 +846,12 @@ def validate_python_install(self, index): code=PYTHON_INVALID, ) python_install['extra_requirements'] = extra_requirements + else: + self.error( + key, + '"path" or "requirements" key is required', + code=CONFIG_REQUIRED, + ) return python_install def get_valid_python_versions(self): diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 378b1bf7cb3..730eb9ebe56 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -1093,6 +1093,12 @@ def test_python_version_check_invalid_types(self, value): build.validate() assert excinfo.value.key == 'python.version' + def test_python_install_default_value(self): + build = self.get_build_config({}) + build.validate() + install = build.python.install + assert len(install) == 0 + def test_python_install_check_default(self, tmpdir): build = self.get_build_config( { @@ -1154,12 +1160,6 @@ def test_python_install_requirements_check_invalid(self, tmpdir): build.validate() assert excinfo.value.key == 'python.install.0.requirements' - def test_python_install_default_value(self): - build = self.get_build_config({}) - build.validate() - install = build.python.install - assert len(install) == 0 - def test_python_install_requirements_does_not_allow_null(self, tmpdir): build = self.get_build_config( { @@ -1237,6 +1237,22 @@ def test_python_install_requirements_check_invalid_types(self, value, tmpdir): build.validate() assert excinfo.value.key == 'python.install.0.requirements' + def test_python_install_path_is_required(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'method': 'pip', + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == 'python.install.0' + assert excinfo.value.code == CONFIG_REQUIRED + def test_python_install_pip_check_valid(self, tmpdir): build = self.get_build_config( { @@ -1407,6 +1423,68 @@ def test_python_install_extra_requirements_allow_empty(self, tmpdir): assert len(install) == 1 assert install[0].extra_requirements == [] + def test_python_install_several_respects_order(self, tmpdir): + apply_fs(tmpdir, { + 'one': {}, + 'two': {}, + 'three.txt': '', + }) + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': 'one', + 'method': 'pip', + 'extra_requirements': [], + }, { + 'path': 'two', + 'method': 'setuptools', + }, { + 'requirements': 'three.txt', + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) + build.validate() + install = build.python.install + assert len(install) == 3 + + assert install[0].path == str(tmpdir.join('one')) + assert install[0].method == PIP + assert install[0].extra_requirements == [] + + assert install[1].path == str(tmpdir.join('two')) + assert install[1].method == SETUPTOOLS + + assert install[2].requirements == str(tmpdir.join('three.txt')) + + def test_python_install_reports_correct_invalid_index(self, tmpdir): + apply_fs(tmpdir, { + 'one': {}, + 'two': {}, + }) + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': 'one', + 'method': 'pip', + 'extra_requirements': [], + }, { + 'path': 'two', + 'method': 'setuptools', + }, { + 'requirements': 'three.txt', + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == 'python.install.2.requirements' + @pytest.mark.parametrize('value', [True, False]) def test_python_system_packages_check_valid(self, value): build = self.get_build_config({ From 897482278a92a9d6b74484d2842353ada30a149d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Oct 2018 12:54:20 -0500 Subject: [PATCH 07/28] Always install from requirements file on v1 --- readthedocs/config/config.py | 9 ++++----- readthedocs/config/tests/test_config.py | 27 +++++++++++++++++-------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 07610faafcb..10d3ba4919d 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -564,12 +564,11 @@ def python(self): python = self._config['python'] requirements = self._config['requirements_file'] python_install = [] - if requirements is not None: - python_install.append( - PythonInstallRequirements( - requirements=requirements, - ) + python_install.append( + PythonInstallRequirements( + requirements=requirements, ) + ) if python['install_with_pip']: python_install.append( PythonInstall( diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 730eb9ebe56..be46ff8ee9f 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -296,16 +296,19 @@ def test_python_pip_install_default(): build.validate() # Default is False. install = build.python.install - assert len(install) == 0 + assert len(install) == 1 + assert not isinstance(install[0], PythonInstall) def describe_validate_python_extra_requirements(): - def it_defaults_to_not_install(): + def it_defaults_to_install_requirements_as_none(): build = get_build_config({'python': {}}, get_env_config()) build.validate() install = build.python.install - assert len(install) == 0 + assert len(install) == 1 + assert isinstance(install[0], PythonInstallRequirements) + assert install[0].requirements is None def it_validates_is_a_list(): build = get_build_config( @@ -367,7 +370,8 @@ def it_defaults_to_false(): build = get_build_config({'python': {}}, get_env_config()) build.validate() install = build.python.install - assert len(install) == 0 + assert len(install) == 1 + assert not isinstance(install[0], PythonInstall) def it_validates_value(): build = get_build_config( @@ -564,7 +568,9 @@ def test_valid_build_config(): assert build.name == 'docs' assert build.base assert build.python - assert len(build.python.install) == 0 + assert len(build.python.install) == 1 + assert isinstance(build.python.install[0], PythonInstallRequirements) + assert build.python.install[0].requirements is None assert build.output_base @@ -735,7 +741,9 @@ def test_validates_conda_file(tmpdir): def test_requirements_file_empty(): build = get_build_config({}, get_env_config()) build.validate() - assert len(build.python.install) == 0 + install = build.python.install + assert len(install) == 1 + assert install[0].requirements is None def test_requirements_file_repects_default_value(tmpdir): @@ -774,7 +782,9 @@ def test_requirements_file_is_null(tmpdir): source_file=str(tmpdir.join('readthedocs.yml')), ) build.validate() - assert len(build.python.install) == 0 + install = build.python.install + assert len(install) == 1 + assert install[0].requirements is None def test_requirements_file_is_blank(tmpdir): @@ -785,7 +795,8 @@ def test_requirements_file_is_blank(tmpdir): ) build.validate() install = build.python.install - assert len(install) == 0 + assert len(install) == 1 + assert install[0].requirements is None def test_build_validate_calls_all_subvalidators(tmpdir): From 74f8470a27f97bc257689113e1e5b815cab972af Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Oct 2018 15:55:30 -0500 Subject: [PATCH 08/28] Always use absolute path --- readthedocs/config/config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 10d3ba4919d..a31133405c2 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -520,7 +520,7 @@ def validate_requirements_file(self): return None base_path = os.path.dirname(self.source_file) with self.catch_validation_error('requirements_file'): - validate_file(requirements_file, base_path) + requirements_file = validate_file(requirements_file, base_path) return requirements_file def validate_formats(self): @@ -577,11 +577,12 @@ def python(self): extra_requirements=python['extra_requirements'], ) ) - if python['install_with_setup']: + elif python['install_with_setup']: python_install.append( PythonInstall( path=self.base_path, method=SETUPTOOLS, + extra_requirements=[], ) ) From bb81a7b74b6c69e8636eb7b2c5d51efc6d19984f Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Oct 2018 15:56:32 -0500 Subject: [PATCH 09/28] Refactor rtd code to use multiple installs --- .../doc_builder/python_environments.py | 57 +++++++++++++------ readthedocs/projects/tasks.py | 3 +- 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index 7d3a4c0e86d..bb5bf902f2c 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -2,18 +2,23 @@ """An abstraction over virtualenv and Conda environments.""" from __future__ import ( - absolute_import, division, print_function, unicode_literals) + absolute_import, + division, + print_function, + unicode_literals, +) import itertools import json import logging import os import shutil -from builtins import object, open import six -from django.conf import settings +from builtins import object, open +from readthedocs.config import PIP, SETUPTOOLS +from readthedocs.config.models import PythonInstall, PythonInstallRequirements from readthedocs.doc_builder.config import load_yaml_config from readthedocs.doc_builder.constants import DOCKER_IMAGE from readthedocs.doc_builder.environments import DockerBuildEnvironment @@ -63,13 +68,23 @@ def delete_existing_venv_dir(self): )) shutil.rmtree(venv_dir) - def install_package(self): - if (self.config.python.install_with_pip or - getattr(settings, 'USE_PIP_INSTALL', False)): + def install_requirements(self): + for install in self.config.python.install: + self._run_install_step(install) + + def _run_install_step(self, install): + if isinstance(install, PythonInstallRequirements): + return self.install_requirements_file(install) + elif isinstance(install, PythonInstall): + return self.install_package(install) + + def install_package(self, install): + rel_path = os.path.relpath(install.path, self.checkout_path) + if install.method == PIP: extra_req_param = '' - if self.config.python.extra_requirements: - extra_req_param = '[{0}]'.format( - ','.join(self.config.python.extra_requirements) + if install.extra_requirements: + extra_req_param = '[{}]'.format( + ','.join(install.extra_requirements) ) self.build_env.run( 'python', @@ -78,14 +93,17 @@ def install_package(self): '--ignore-installed', '--cache-dir', self.project.pip_cache_path, - '.{0}'.format(extra_req_param), + '{path}{extra_requirements}'.format( + path=rel_path, + extra_requirements=extra_req_param, + ), cwd=self.checkout_path, bin_path=self.venv_bin(), ) - elif self.config.python.install_with_setup: + elif install.method == SETUPTOOLS: self.build_env.run( 'python', - 'setup.py', + os.path.join(rel_path, 'setup.py'), 'install', '--force', cwd=self.checkout_path, @@ -274,9 +292,11 @@ def install_core_requirements(self): cwd=self.checkout_path # noqa - no comma here in py27 :/ ) - def install_user_requirements(self): - requirements_file_path = self.config.python.requirements - if not requirements_file_path and requirements_file_path != '': + def install_requirements_file(self, install): + requirements_file_path = install.requirements + if requirements_file_path is None: + # This only happens when the config file is from v1. + # We try to find a requirements file. builder_class = get_builder_class(self.config.doctype) docs_dir = (builder_class(build_env=self.build_env, python_env=self) .docs_dir()) @@ -301,7 +321,10 @@ def install_user_requirements(self): '--cache-dir', self.project.pip_cache_path, '-r', - requirements_file_path, + os.path.relpath( + requirements_file_path, + self.checkout_path + ), ] self.build_env.run( *args, @@ -392,7 +415,7 @@ def install_core_requirements(self): cwd=self.checkout_path # noqa - no comma here in py27 :/ ) - def install_user_requirements(self): + def install_requirements_file(self, install): # as the conda environment was created by using the ``environment.yml`` # defined by the user, there is nothing to update at this point pass diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index c82b561219a..07a2a21672a 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -664,8 +664,7 @@ def setup_python_environment(self): self.python_env.setup_base() self.python_env.save_environment_json() self.python_env.install_core_requirements() - self.python_env.install_user_requirements() - self.python_env.install_package() + self.python_env.install_requirements() def build_docs(self): """ From 6897f07fd8f07567cb0bb668b157233797bb6314 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Oct 2018 17:49:46 -0500 Subject: [PATCH 10/28] Fix tests --- readthedocs/config/tests/test_config.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index be46ff8ee9f..9168fcce768 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -12,14 +12,14 @@ from readthedocs.config import ( ALL, + PIP, + SETUPTOOLS, BuildConfigV1, BuildConfigV2, ConfigError, ConfigOptionNotSupportedError, InvalidConfig, ProjectConfig, - PIP, - SETUPTOOLS, load, ) from readthedocs.config.config import ( @@ -32,7 +32,11 @@ PYTHON_INVALID, VERSION_INVALID, ) -from readthedocs.config.models import Conda, PythonInstall, PythonInstallRequirements +from readthedocs.config.models import ( + Conda, + PythonInstall, + PythonInstallRequirements, +) from readthedocs.config.validation import ( INVALID_BOOL, INVALID_CHOICE, @@ -759,7 +763,7 @@ def test_requirements_file_repects_default_value(tmpdir): build.validate() install = build.python.install assert len(install) == 1 - assert install[0].requirements == 'myrequirements.txt' + assert install[0].requirements == str(tmpdir.join('myrequirements.txt')) def test_requirements_file_respects_configuration(tmpdir): @@ -772,7 +776,7 @@ def test_requirements_file_respects_configuration(tmpdir): build.validate() install = build.python.install assert len(install) == 1 - assert install[0].requirements == 'requirements.txt' + assert install[0].requirements == str(tmpdir.join('requirements.txt')) def test_requirements_file_is_null(tmpdir): From 2987a333d9d5ae04b75ad479bd2d1185f55b0a46 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Oct 2018 19:14:29 -0500 Subject: [PATCH 11/28] Fix tests from config_integration --- readthedocs/config/config.py | 10 +- .../tests/test_config_integration.py | 171 +++++++++++------- 2 files changed, 115 insertions(+), 66 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index a31133405c2..f1bd7972a90 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -1091,7 +1091,8 @@ def build(self): @property def python(self): python_install = [] - for install in self._config['python']['install']: + python = self._config['python'] + for install in python['install']: if 'requirements' in install: python_install.append( PythonInstallRequirements(**install) @@ -1100,8 +1101,11 @@ def python(self): python_install.append( PythonInstall(**install) ) - self._config['python']['install'] = python_install - return Python(**self._config['python']) + return Python( + version=python['version'], + install=python_install, + use_system_site_packages=python['use_system_site_packages'], + ) @property def sphinx(self): diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index cc49317dad8..d1283d5656b 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -1,6 +1,11 @@ # -*- coding: utf-8 -*- + from __future__ import ( - absolute_import, division, print_function, unicode_literals) + absolute_import, + division, + print_function, + unicode_literals, +) import tempfile from os import path @@ -13,7 +18,15 @@ from mock import MagicMock, PropertyMock, patch from readthedocs.builds.models import Version -from readthedocs.config import ALL, BuildConfigV1, InvalidConfig, ProjectConfig +from readthedocs.config import ( + ALL, + PIP, + SETUPTOOLS, + BuildConfigV1, + InvalidConfig, + ProjectConfig, +) +from readthedocs.config.models import PythonInstallRequirements from readthedocs.config.tests.utils import apply_fs from readthedocs.doc_builder.config import load_yaml_config from readthedocs.doc_builder.environments import LocalBuildEnvironment @@ -183,16 +196,23 @@ def test_python_invalid_version_in_config(self, load_config): def test_install_project(self, load_config): load_config.side_effect = create_load() config = load_yaml_config(self.version) - self.assertEqual( - config.python.install_with_pip or config.python.install_with_setup, - False + self.assertEqual(len(config.python.install), 1) + self.assertTrue( + isinstance(config.python.install[0], PythonInstallRequirements) ) load_config.side_effect = create_load({ 'python': {'setup_py_install': True} }) config = load_yaml_config(self.version) - self.assertEqual(config.python.install_with_setup, True) + self.assertEqual(len(config.python.install), 2) + self.assertTrue( + isinstance(config.python.install[0], PythonInstallRequirements) + ) + self.assertEqual( + config.python.install[1].method, + SETUPTOOLS + ) @mock.patch('readthedocs.doc_builder.config.load_config') def test_extra_requirements(self, load_config): @@ -203,7 +223,14 @@ def test_extra_requirements(self, load_config): } }) config = load_yaml_config(self.version) - self.assertEqual(config.python.extra_requirements, ['tests', 'docs']) + self.assertEqual(len(config.python.install), 2) + self.assertTrue( + isinstance(config.python.install[0], PythonInstallRequirements) + ) + self.assertEqual( + config.python.install[1].extra_requirements, + ['tests', 'docs'] + ) load_config.side_effect = create_load({ 'python': { @@ -211,11 +238,17 @@ def test_extra_requirements(self, load_config): } }) config = load_yaml_config(self.version) - self.assertEqual(config.python.extra_requirements, []) + self.assertEqual(len(config.python.install), 1) + self.assertTrue( + isinstance(config.python.install[0], PythonInstallRequirements) + ) load_config.side_effect = create_load() config = load_yaml_config(self.version) - self.assertEqual(config.python.extra_requirements, []) + self.assertEqual(len(config.python.install), 1) + self.assertTrue( + isinstance(config.python.install[0], PythonInstallRequirements) + ) load_config.side_effect = create_load({ 'python': { @@ -224,7 +257,14 @@ def test_extra_requirements(self, load_config): } }) config = load_yaml_config(self.version) - self.assertEqual(config.python.extra_requirements, []) + self.assertEqual(len(config.python.install), 2) + self.assertTrue( + isinstance(config.python.install[0], PythonInstallRequirements) + ) + self.assertEqual( + config.python.install[1].extra_requirements, + [] + ) @mock.patch('readthedocs.projects.models.Project.checkout_path') def test_conda_with_cofig(self, checkout_path): @@ -267,7 +307,11 @@ def test_requirements_file_from_project_setting(self, checkout_path): f.write('pip') config = load_yaml_config(self.version) - self.assertEqual(config.python.requirements, requirements_file) + self.assertEqual(len(config.python.install), 1) + self.assertEqual( + config.python.install[0].requirements, + full_requirements_file + ) @mock.patch('readthedocs.projects.models.Project.checkout_path') def test_requirements_file_from_yml(self, checkout_path): @@ -288,7 +332,11 @@ def test_requirements_file_from_yml(self, checkout_path): base_path=base_path, ) config = load_yaml_config(self.version) - self.assertEqual(config.python.requirements, requirements_file) + self.assertEqual(len(config.python.install), 1) + self.assertEqual( + config.python.install[0].requirements, + full_requirements_file + ) @pytest.mark.django_db @@ -466,15 +514,19 @@ def test_python_version(self, checkout_path, tmpdir): assert config.python_full_version == 3.6 @patch('readthedocs.doc_builder.environments.BuildEnvironment.run') - def test_python_requirements(self, run, checkout_path, tmpdir): + def test_python_install_requirements(self, run, checkout_path, tmpdir): checkout_path.return_value = str(tmpdir) requirements_file = 'requirements.txt' apply_fs(tmpdir, {requirements_file: ''}) base_path = self.create_config_file( tmpdir, { - 'python': {'requirements': requirements_file} - } + 'python': { + 'install': [{ + 'requirements': requirements_file + }], + }, + }, ) update_docs = self.get_update_docs_task() @@ -486,45 +538,28 @@ def test_python_requirements(self, run, checkout_path, tmpdir): config=config ) update_docs.python_env = python_env - update_docs.python_env.install_user_requirements() + update_docs.python_env.install_requirements() args, kwargs = run.call_args - requirements_file = path.join(str(base_path), requirements_file) + full_requirements_file = str(base_path.join(requirements_file)) + install = config.python.install - assert config.python.requirements == requirements_file + assert len(install) == 1 + assert install[0].requirements == full_requirements_file assert requirements_file in args @patch('readthedocs.doc_builder.environments.BuildEnvironment.run') - def test_python_requirements_empty(self, run, checkout_path, tmpdir): - checkout_path.return_value = str(tmpdir) - self.create_config_file( - tmpdir, - { - 'python': {'requirements': ''} - } - ) - - update_docs = self.get_update_docs_task() - config = update_docs.config - - python_env = Virtualenv( - version=self.version, - build_env=update_docs.build_env, - config=config - ) - update_docs.python_env = python_env - update_docs.python_env.install_user_requirements() - - assert config.python.requirements == '' - assert not run.called - - @patch('readthedocs.doc_builder.environments.BuildEnvironment.run') - def test_python_install_setup(self, run, checkout_path, tmpdir): + def test_python_install_setuptools(self, run, checkout_path, tmpdir): checkout_path.return_value = str(tmpdir) self.create_config_file( tmpdir, { - 'python': {'install': 'setup.py'} + 'python': { + 'install': [{ + 'path': '.', + 'method': 'setuptools', + }], + }, } ) @@ -537,14 +572,15 @@ def test_python_install_setup(self, run, checkout_path, tmpdir): config=config ) update_docs.python_env = python_env - update_docs.python_env.install_package() + update_docs.python_env.install_requirements() args, kwargs = run.call_args - assert 'setup.py' in args + assert './setup.py' in args assert 'install' in args - assert config.python.install_with_setup - assert not config.python.install_with_pip + install = config.python.install + assert len(install) == 1 + assert install[0].method == SETUPTOOLS @patch('readthedocs.doc_builder.environments.BuildEnvironment.run') def test_python_install_pip(self, run, checkout_path, tmpdir): @@ -552,7 +588,12 @@ def test_python_install_pip(self, run, checkout_path, tmpdir): self.create_config_file( tmpdir, { - 'python': {'install': 'pip'} + 'python': { + 'install': [{ + 'path': '.', + 'method': 'pip', + }], + }, } ) @@ -565,14 +606,15 @@ def test_python_install_pip(self, run, checkout_path, tmpdir): config=config ) update_docs.python_env = python_env - update_docs.python_env.install_package() + update_docs.python_env.install_requirements() args, kwargs = run.call_args - assert 'setup.py' not in args + assert './setup.py' not in args assert 'install' in args - assert config.python.install_with_pip - assert not config.python.install_with_setup + install = config.python.install + assert len(install) == 1 + assert install[0].method == PIP def test_python_install_project(self, checkout_path, tmpdir): checkout_path.return_value = str(tmpdir) @@ -583,19 +625,21 @@ def test_python_install_project(self, checkout_path, tmpdir): config = self.get_update_docs_task().config - assert config.python.install_with_setup - assert not config.python.install_with_pip + assert config.python.install == [] @patch('readthedocs.doc_builder.environments.BuildEnvironment.run') - def test_python_extra_requirements(self, run, checkout_path, tmpdir): + def test_python_install_extra_requirements(self, run, checkout_path, tmpdir): checkout_path.return_value = str(tmpdir) self.create_config_file( tmpdir, { 'python': { - 'install': 'pip', - 'extra_requirements': ['docs'], - } + 'install': [{ + 'path': '.', + 'method': 'pip', + 'extra_requirements': ['docs'], + }], + }, } ) @@ -608,15 +652,16 @@ def test_python_extra_requirements(self, run, checkout_path, tmpdir): config=config ) update_docs.python_env = python_env - update_docs.python_env.install_package() + update_docs.python_env.install_requirements() args, kwargs = run.call_args - assert 'setup.py' not in args + assert './setup.py' not in args assert 'install' in args assert '.[docs]' in args - assert config.python.install_with_pip - assert not config.python.install_with_setup + install = config.python.install + assert len(install) == 1 + assert install[0].method == PIP @patch('readthedocs.doc_builder.environments.BuildEnvironment.run') def test_system_packages(self, run, checkout_path, tmpdir): From 4b7ca7f04adbdfb38cc0b3addaf836b7f816799a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Oct 2018 19:32:53 -0500 Subject: [PATCH 12/28] Fix test for doc_building --- readthedocs/rtd_tests/tests/test_doc_building.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index d85e3a07038..6f5155a091a 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -1259,8 +1259,8 @@ def test_install_user_requirements(self, checkout_path): paths[docs_requirements] = True paths[root_requirements] = False with fake_paths_lookup(paths): - python_env.install_user_requirements() - args[-1] = docs_requirements + python_env.install_requirements() + args[-1] = 'docs/requirements.txt' self.build_env_mock.run.assert_called_with( *args, cwd=mock.ANY, bin_path=mock.ANY ) @@ -1270,8 +1270,8 @@ def test_install_user_requirements(self, checkout_path): paths[docs_requirements] = False paths[root_requirements] = True with fake_paths_lookup(paths): - python_env.install_user_requirements() - args[-1] = root_requirements + python_env.install_requirements() + args[-1] = 'requirements.txt' self.build_env_mock.run.assert_called_with( *args, cwd=mock.ANY, bin_path=mock.ANY ) @@ -1281,8 +1281,8 @@ def test_install_user_requirements(self, checkout_path): paths[docs_requirements] = True paths[root_requirements] = True with fake_paths_lookup(paths): - python_env.install_user_requirements() - args[-1] = docs_requirements + python_env.install_requirements() + args[-1] = 'docs/requirements.txt' self.build_env_mock.run.assert_called_with( *args, cwd=mock.ANY, bin_path=mock.ANY ) @@ -1293,7 +1293,7 @@ def test_install_user_requirements(self, checkout_path): paths[docs_requirements] = False paths[root_requirements] = False with fake_paths_lookup(paths): - python_env.install_user_requirements() + python_env.install_requirements() self.build_env_mock.run.assert_not_called() @patch('readthedocs.projects.models.Project.checkout_path') @@ -1386,7 +1386,7 @@ def test_install_user_requirements_conda(self, checkout_path): version=self.version_sphinx, build_env=self.build_env_mock, ) - python_env.install_user_requirements() + python_env.install_requirements() self.build_env_mock.run.assert_not_called() From 3fb59648bd5384c54765ba44cd419dc1989eae4e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Oct 2018 20:07:06 -0500 Subject: [PATCH 13/28] Fix linter --- readthedocs/config/config.py | 18 ++++++++++-------- readthedocs/doc_builder/python_environments.py | 2 +- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index f1bd7972a90..7516f00a25d 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -93,6 +93,14 @@ } +def _list_to_dict(list_): + dict_ = { + str(i): element + for i, element in enumerate(list_) + } + return dict_ + + class ConfigError(Exception): """Base error for the rtd configuration file.""" @@ -770,7 +778,7 @@ def validate_python(self): validate_list(raw_install) if raw_install: self.raw_config.setdefault('python', {})['install'] = ( - self._list_to_dict(raw_install) + _list_to_dict(raw_install) ) else: self.pop_config('python.install') @@ -794,14 +802,8 @@ def validate_python(self): return python - def _list_to_dict(self, list_): - dict_ = { - str(i): element - for i, element in enumerate(list_) - } - return dict_ - def validate_python_install(self, index): + """Validates the python.install.index key.""" python_install = {} key = 'python.install.{}'.format(index) raw_install = self.raw_config['python']['install'][str(index)] diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index bb5bf902f2c..add1513701a 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -75,7 +75,7 @@ def install_requirements(self): def _run_install_step(self, install): if isinstance(install, PythonInstallRequirements): return self.install_requirements_file(install) - elif isinstance(install, PythonInstall): + if isinstance(install, PythonInstall): return self.install_package(install) def install_package(self, install): From 789dff356a16b07bf58bdd29549d74fc59fea73b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 10 Oct 2018 12:18:33 -0500 Subject: [PATCH 14/28] New test for multiple installs --- .../doc_builder/python_environments.py | 1 + .../tests/test_config_integration.py | 61 ++++++++++++++++++- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index add1513701a..8b0695854be 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -93,6 +93,7 @@ def install_package(self, install): '--ignore-installed', '--cache-dir', self.project.pip_cache_path, + '-e', '{path}{extra_requirements}'.format( path=rel_path, extra_requirements=extra_req_param, diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index d1283d5656b..979a4f1783f 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -610,8 +610,9 @@ def test_python_install_pip(self, run, checkout_path, tmpdir): args, kwargs = run.call_args - assert './setup.py' not in args assert 'install' in args + assert '-e' in args + assert '.' in args install = config.python.install assert len(install) == 1 assert install[0].method == PIP @@ -656,13 +657,69 @@ def test_python_install_extra_requirements(self, run, checkout_path, tmpdir): args, kwargs = run.call_args - assert './setup.py' not in args assert 'install' in args + assert '-e' in args assert '.[docs]' in args install = config.python.install assert len(install) == 1 assert install[0].method == PIP + @patch('readthedocs.doc_builder.environments.BuildEnvironment.run') + def test_python_install_several_options(self, run, checkout_path, tmpdir): + checkout_path.return_value = str(tmpdir) + apply_fs(tmpdir, { + 'one': {}, + 'two': {}, + 'three.txt': '', + }) + self.create_config_file( + tmpdir, + { + 'python': { + 'install': [{ + 'path': 'one', + 'method': 'pip', + 'extra_requirements': ['docs'], + }, { + 'path': 'two', + 'method': 'setuptools', + }, { + 'requirements': 'three.txt', + }], + }, + } + ) + + update_docs = self.get_update_docs_task() + config = update_docs.config + + python_env = Virtualenv( + version=self.version, + build_env=update_docs.build_env, + config=config + ) + update_docs.python_env = python_env + update_docs.python_env.install_requirements() + + install = config.python.install + assert len(install) == 3 + + args, kwargs = run.call_args_list[0] + assert 'install' in args + assert '-e' in args + assert 'one[docs]' in args + assert install[0].method == PIP + + args, kwargs = run.call_args_list[1] + assert 'two/setup.py' in args + assert 'install' in args + assert install[1].method == SETUPTOOLS + + args, kwargs = run.call_args_list[2] + assert 'install' in args + assert '-r' in args + assert 'three.txt' in args + @patch('readthedocs.doc_builder.environments.BuildEnvironment.run') def test_system_packages(self, run, checkout_path, tmpdir): checkout_path.return_value = str(tmpdir) From 058fadd187d6acfbb05ddd6937f14a74e0b9c66c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 10 Oct 2018 12:55:45 -0500 Subject: [PATCH 15/28] Install from local path --- readthedocs/doc_builder/python_environments.py | 7 +++++-- readthedocs/rtd_tests/tests/test_config_integration.py | 5 +---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index 8b0695854be..d53b73a9839 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -81,6 +81,10 @@ def _run_install_step(self, install): def install_package(self, install): rel_path = os.path.relpath(install.path, self.checkout_path) if install.method == PIP: + # Prefix ./ so pip installs from a local path rather than pypi + local_path = ( + os.path.join('.', rel_path) if rel_path != '.' else rel_path + ) extra_req_param = '' if install.extra_requirements: extra_req_param = '[{}]'.format( @@ -93,9 +97,8 @@ def install_package(self, install): '--ignore-installed', '--cache-dir', self.project.pip_cache_path, - '-e', '{path}{extra_requirements}'.format( - path=rel_path, + path=local_path, extra_requirements=extra_req_param, ), cwd=self.checkout_path, diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index 979a4f1783f..5b579a41b75 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -611,7 +611,6 @@ def test_python_install_pip(self, run, checkout_path, tmpdir): args, kwargs = run.call_args assert 'install' in args - assert '-e' in args assert '.' in args install = config.python.install assert len(install) == 1 @@ -658,7 +657,6 @@ def test_python_install_extra_requirements(self, run, checkout_path, tmpdir): args, kwargs = run.call_args assert 'install' in args - assert '-e' in args assert '.[docs]' in args install = config.python.install assert len(install) == 1 @@ -706,8 +704,7 @@ def test_python_install_several_options(self, run, checkout_path, tmpdir): args, kwargs = run.call_args_list[0] assert 'install' in args - assert '-e' in args - assert 'one[docs]' in args + assert './one[docs]' in args assert install[0].method == PIP args, kwargs = run.call_args_list[1] From 46c2ffb54e7df72027972b6b53c08db87387b387 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 10 Oct 2018 13:49:19 -0500 Subject: [PATCH 16/28] Refactoring and docstrings --- readthedocs/config/config.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 7516f00a25d..9abf97786c6 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -94,6 +94,7 @@ def _list_to_dict(list_): + """Transform a list to a dictionary with its indices as keys.""" dict_ = { str(i): element for i, element in enumerate(list_) @@ -572,6 +573,9 @@ def python(self): python = self._config['python'] requirements = self._config['requirements_file'] python_install = [] + + # Alwyas append a `PythonInstallRequirements` option. + # If requirements is None, rtd will try to find a requirements file. python_install.append( PythonInstallRequirements( requirements=requirements, @@ -777,6 +781,7 @@ def validate_python(self): raw_install = self.raw_config.get('python', {}).get('install', []) validate_list(raw_install) if raw_install: + # Transform to a dict, so it's easy to validate extra keys. self.raw_config.setdefault('python', {})['install'] = ( _list_to_dict(raw_install) ) @@ -803,7 +808,7 @@ def validate_python(self): return python def validate_python_install(self, index): - """Validates the python.install.index key.""" + """Validates the python.install.{index} key.""" python_install = {} key = 'python.install.{}'.format(index) raw_install = self.raw_config['python']['install'][str(index)] @@ -835,14 +840,14 @@ def validate_python_install(self, index): ) python_install['method'] = method - extrareq_key = key + '.extra_requirements' - with self.catch_validation_error(extrareq_key): + extra_req_key = key + '.extra_requirements' + with self.catch_validation_error(extra_req_key): extra_requirements = validate_list( - self.pop_config(extrareq_key, []) + self.pop_config(extra_req_key, []) ) if extra_requirements and python_install['method'] != PIP: self.error( - extrareq_key, + extra_req_key, 'You need to install your project with pip ' 'to use extra_requirements', code=PYTHON_INVALID, From 39d6e3b8973fc8a11490747f50e9b51288565669 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 10 Oct 2018 13:49:43 -0500 Subject: [PATCH 17/28] Move comment --- readthedocs/doc_builder/python_environments.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index d53b73a9839..9e94e106b4e 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -298,9 +298,9 @@ def install_core_requirements(self): def install_requirements_file(self, install): requirements_file_path = install.requirements + # This only happens when the config file is from v1. + # We try to find a requirements file. if requirements_file_path is None: - # This only happens when the config file is from v1. - # We try to find a requirements file. builder_class = get_builder_class(self.config.doctype) docs_dir = (builder_class(build_env=self.build_env, python_env=self) .docs_dir()) From f99afe4e29183bad322276f6aeecde1516065fe6 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 10 Oct 2018 13:49:55 -0500 Subject: [PATCH 18/28] Mores tests --- readthedocs/config/tests/test_config.py | 46 +++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 9168fcce768..07f9c4d7ed5 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -1133,12 +1133,38 @@ def test_python_install_check_default(self, tmpdir): assert install[0].method == PIP assert install[0].extra_requirements == [] + def test_python_install_path_check_invalid(self, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': 'noexists', + 'method': 'pip', + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == 'python.install.0.path' + @pytest.mark.parametrize('value', ['invalid', 'apt']) - def test_python_install_check_invalid(self, value): - build = self.get_build_config({'python': {'install': value}},) + def test_python_install_method_check_invalid(self, value, tmpdir): + build = self.get_build_config( + { + 'python': { + 'install': [{ + 'path': '.', + 'method': value, + }], + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) with raises(InvalidConfig) as excinfo: build.validate() - assert excinfo.value.key == 'python.install' + assert excinfo.value.key == 'python.install.0.method' def test_python_install_requirements_check_valid(self, tmpdir): apply_fs(tmpdir, {'requirements.txt': ''}) @@ -1973,6 +1999,20 @@ def test_submodules_recursive_explict_default(self): } }, 'build.extra' + ), + ( + { + 'python': { + 'install': [{ + 'path': '.', + }, { + 'path': '.', + 'method': 'pip', + 'invalid': 'key', + }] + } + }, + 'python.install.1.invalid' ) ]) def test_strict_validation(self, value, key): From fdf84a04c50bb7ff82251c515146231dce6aad68 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 14 Nov 2018 18:01:44 -0500 Subject: [PATCH 19/28] Use classes instead of namedtuples for config models We need to have a custom `as_dict` method for models that contain other models inside --- readthedocs/config/config.py | 20 +++- readthedocs/config/models.py | 121 +++++++++++++++--------- readthedocs/config/tests/test_config.py | 18 ++-- 3 files changed, 100 insertions(+), 59 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 861fea7948f..c0d68056470 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -270,12 +270,24 @@ def as_dict(self): config = {} for name in self.PUBLIC_ATTRIBUTES: attr = getattr(self, name) - if hasattr(attr, '_asdict'): - config[name] = attr._asdict() - else: - config[name] = attr + config[name] = self.to_dict(attr) return config + def to_dict(self, value): + if hasattr(value, 'as_dict'): + return value.as_dict() + if isinstance(value, list): + return [ + self.to_dict(e) + for e in value + ] + if isinstance(value, dict): + return { + k: self.to_dict(v) + for k, v in value.items() + } + return value + def __getattr__(self, name): """Raise an error for unknown attributes.""" raise ConfigOptionNotSupportedError(name) diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index 5efe590a189..536078106ad 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -2,49 +2,78 @@ from __future__ import division, print_function, unicode_literals -from collections import namedtuple - - -Build = namedtuple('Build', ['image']) # noqa - -Python = namedtuple( # noqa - 'Python', - [ - 'version', - 'install', - 'use_system_site_packages', - ], -) - -PythonInstallRequirements = namedtuple( - 'PythonInstallRequirements', - [ - 'requirements', - ], -) - -PythonInstall = namedtuple( - 'PythonInstall', - [ - 'path', - 'method', - 'extra_requirements', - ], -) - -Conda = namedtuple('Conda', ['environment']) # noqa - -Sphinx = namedtuple( # noqa - 'Sphinx', - ['builder', 'configuration', 'fail_on_warning'], -) - -Mkdocs = namedtuple( # noqa - 'Mkdocs', - ['configuration', 'fail_on_warning'], -) - -Submodules = namedtuple( # noqa - 'Submodules', - ['include', 'exclude', 'recursive'], -) + +class Base(object): + + """ + Base class for every configuration. + + Each inherited class should define + its attibutes in the `__slots__` attribute. + """ + + def __init__(self, **kwargs): + for name in self.__slots__: + setattr(self, name, kwargs[name]) + + def as_dict(self): + return { + name: self.to_dict(getattr(self, name)) + for name in self.__slots__ + } + + def to_dict(self, value): + """Recursively transform the class to a dict.""" + if hasattr(value, 'as_dict'): + return value.as_dict() + if isinstance(value, list): + return [ + self.to_dict(e) + for e in value + ] + if isinstance(value, dict): + return { + k: self.to_dict(v) + for k, v in value.items() + } + return value + + +class Build(Base): + + __slots__ = ('image',) + + +class Python(Base): + + __slots__ = ('version', 'install', 'use_system_site_packages') + + +class PythonInstallRequirements(Base): + + __slots__ = ('requirements',) + + +class PythonInstall(Base): + + __slots__ = ('path', 'method', 'extra_requirements',) + + +class Conda(Base): + + __slots__ = ('environment',) + + +class Sphinx(Base): + + __slots__ = ('builder', 'configuration', 'fail_on_warning') + + +class Mkdocs(Base): + + __slots__ = ('configuration', 'fail_on_warning') + + +class Submodules(Base): + + __slots__ = ('include', 'exclude', 'recursive') diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index d28ce75fbeb..b0be6009cdf 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -835,10 +835,9 @@ def test_as_dict(tmpdir): 'formats': ['pdf'], 'python': { 'version': 3.5, - 'requirements': 'requirements.txt', - 'install_with_pip': False, - 'install_with_setup': False, - 'extra_requirements': [], + 'install': [{ + 'requirements': str(tmpdir.join('requirements.txt')), + }], 'use_system_site_packages': False, }, 'build': { @@ -2091,7 +2090,9 @@ def test_as_dict(self, tmpdir): 'formats': ['pdf'], 'python': { 'version': 3.6, - 'requirements': 'requirements.txt', + 'install': [{ + 'requirements': 'requirements.txt', + }], }, }, source_file=str(tmpdir.join('readthedocs.yml')), @@ -2102,10 +2103,9 @@ def test_as_dict(self, tmpdir): 'formats': ['pdf'], 'python': { 'version': 3.6, - 'requirements': str(tmpdir.join('requirements.txt')), - 'install_with_pip': False, - 'install_with_setup': False, - 'extra_requirements': [], + 'install': [{ + 'requirements': str(tmpdir.join('requirements.txt')), + }], 'use_system_site_packages': False, }, 'build': { From 8f2f89cd1c3f9a74500c9dd980abfbba51b800a3 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 14 Nov 2018 21:13:45 -0500 Subject: [PATCH 20/28] Move to_dict to utils --- readthedocs/config/config.py | 18 ++---------------- readthedocs/config/models.py | 20 +++----------------- readthedocs/config/utils.py | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 33 deletions(-) create mode 100644 readthedocs/config/utils.py diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index c0d68056470..6ab709d5aa4 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -11,6 +11,7 @@ import six +from readthedocs.config.utils import to_dict from readthedocs.projects.constants import DOCUMENTATION_CHOICES from .find import find_one @@ -270,24 +271,9 @@ def as_dict(self): config = {} for name in self.PUBLIC_ATTRIBUTES: attr = getattr(self, name) - config[name] = self.to_dict(attr) + config[name] = to_dict(attr) return config - def to_dict(self, value): - if hasattr(value, 'as_dict'): - return value.as_dict() - if isinstance(value, list): - return [ - self.to_dict(e) - for e in value - ] - if isinstance(value, dict): - return { - k: self.to_dict(v) - for k, v in value.items() - } - return value - def __getattr__(self, name): """Raise an error for unknown attributes.""" raise ConfigOptionNotSupportedError(name) diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index 536078106ad..944b3e2135c 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -2,6 +2,8 @@ from __future__ import division, print_function, unicode_literals +from readthedocs.config.utils import to_dict + class Base(object): @@ -18,26 +20,10 @@ def __init__(self, **kwargs): def as_dict(self): return { - name: self.to_dict(getattr(self, name)) + name: to_dict(getattr(self, name)) for name in self.__slots__ } - def to_dict(self, value): - """Recursively transform the class to a dict.""" - if hasattr(value, 'as_dict'): - return value.as_dict() - if isinstance(value, list): - return [ - self.to_dict(e) - for e in value - ] - if isinstance(value, dict): - return { - k: self.to_dict(v) - for k, v in value.items() - } - return value - class Build(Base): diff --git a/readthedocs/config/utils.py b/readthedocs/config/utils.py new file mode 100644 index 00000000000..126350cba5e --- /dev/null +++ b/readthedocs/config/utils.py @@ -0,0 +1,18 @@ +"""Shared functions for the config module.""" + + +def to_dict(value): + """Recursively transform a class from `config.models` to a dict.""" + if hasattr(value, 'as_dict'): + return value.as_dict() + if isinstance(value, list): + return [ + to_dict(v) + for v in value + ] + if isinstance(value, dict): + return { + k: to_dict(v) + for k, v in value.items() + } + return value From a6b7c9715fb7bf8a2ace0384df1f4c61bff28c5c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 5 Dec 2018 12:38:14 -0500 Subject: [PATCH 21/28] Move function to utils --- readthedocs/config/config.py | 13 ++----------- readthedocs/config/utils.py | 9 +++++++++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 6ab709d5aa4..ce3e16b1bfd 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -11,7 +11,7 @@ import six -from readthedocs.config.utils import to_dict +from readthedocs.config.utils import to_dict, list_to_dict from readthedocs.projects.constants import DOCUMENTATION_CHOICES from .find import find_one @@ -93,15 +93,6 @@ } -def _list_to_dict(list_): - """Transform a list to a dictionary with its indices as keys.""" - dict_ = { - str(i): element - for i, element in enumerate(list_) - } - return dict_ - - class ConfigError(Exception): """Base error for the rtd configuration file.""" @@ -800,7 +791,7 @@ def validate_python(self): if raw_install: # Transform to a dict, so it's easy to validate extra keys. self.raw_config.setdefault('python', {})['install'] = ( - _list_to_dict(raw_install) + list_to_dict(raw_install) ) else: self.pop_config('python.install') diff --git a/readthedocs/config/utils.py b/readthedocs/config/utils.py index 126350cba5e..abec9aded26 100644 --- a/readthedocs/config/utils.py +++ b/readthedocs/config/utils.py @@ -16,3 +16,12 @@ def to_dict(value): for k, v in value.items() } return value + + +def list_to_dict(list_): + """Transform a list to a dictionary with its indices as keys.""" + dict_ = { + str(i): element + for i, element in enumerate(list_) + } + return dict_ From d1941980e48e738eaf6231a630595d85eeadf390 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 5 Dec 2018 12:41:01 -0500 Subject: [PATCH 22/28] Add explanation about using __slots__ --- readthedocs/config/models.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index 944b3e2135c..fe841c4a252 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -12,6 +12,9 @@ class Base(object): Each inherited class should define its attibutes in the `__slots__` attribute. + + We are using `__slots__` so we can't add more attributes by mistake, + this is similar to a namedtuple. """ def __init__(self, **kwargs): From b55525908b8874338e484f5b019c5ef64d72e441 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 5 Dec 2018 13:19:07 -0500 Subject: [PATCH 23/28] Docstrings --- readthedocs/config/config.py | 2 +- readthedocs/doc_builder/python_environments.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index ce3e16b1bfd..86d34f65d91 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -582,7 +582,7 @@ def python(self): requirements = self._config['requirements_file'] python_install = [] - # Alwyas append a `PythonInstallRequirements` option. + # Always append a `PythonInstallRequirements` option. # If requirements is None, rtd will try to find a requirements file. python_install.append( PythonInstallRequirements( diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index 2b9468046be..a1dab7d7de7 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -70,16 +70,15 @@ def delete_existing_venv_dir(self): shutil.rmtree(venv_dir) def install_requirements(self): + """Install all requirements from the config object.""" for install in self.config.python.install: - self._run_install_step(install) - - def _run_install_step(self, install): - if isinstance(install, PythonInstallRequirements): - return self.install_requirements_file(install) - if isinstance(install, PythonInstall): - return self.install_package(install) + if isinstance(install, PythonInstallRequirements): + return self.install_requirements_file(install) + if isinstance(install, PythonInstall): + return self.install_package(install) def install_package(self, install): + """Install the package using pip or setuptools.""" rel_path = os.path.relpath(install.path, self.checkout_path) if install.method == PIP: # Prefix ./ so pip installs from a local path rather than pypi @@ -311,6 +310,7 @@ def install_core_requirements(self): ) def install_requirements_file(self, install): + """Install a requirements file using pip.""" requirements_file_path = install.requirements # This only happens when the config file is from v1. # We try to find a requirements file. From 9c677a4e611d6328313e9eeca8f2bbcd876c105c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 5 Dec 2018 13:19:25 -0500 Subject: [PATCH 24/28] Better tests --- readthedocs/config/tests/test_config.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index b0be6009cdf..73fbb48134f 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -267,15 +267,6 @@ def test_use_system_site_packages_repects_default_value(value): assert build.python.use_system_site_packages is value -def test_python_pip_install_default(): - build = get_build_config({'python': {}}, get_env_config()) - build.validate() - # Default is False. - install = build.python.install - assert len(install) == 1 - assert not isinstance(install[0], PythonInstall) - - class TestValidatePythonExtraRequirements(object): def test_it_defaults_to_install_requirements_as_none(self): @@ -347,7 +338,8 @@ def test_it_defaults_to_false(self): build.validate() install = build.python.install assert len(install) == 1 - assert not isinstance(install[0], PythonInstall) + assert isinstance(install[0], PythonInstallRequirements) + assert install[0].requirements is None def test_it_validates_value(self): build = get_build_config( @@ -1193,12 +1185,13 @@ def test_python_install_requirements_check_valid(self, tmpdir): def test_python_install_requirements_check_invalid(self, tmpdir): apply_fs(tmpdir, {'requirements.txt': ''}) + requirements_file = 'invalid' build = self.get_build_config( { 'python': { 'install': [{ 'path': '.', - 'requirements': 'invalid', + 'requirements': requirements_file, }], }, }, @@ -1207,6 +1200,8 @@ def test_python_install_requirements_check_invalid(self, tmpdir): with raises(InvalidConfig) as excinfo: build.validate() assert excinfo.value.key == 'python.install.0.requirements' + error_msg = 'path {} does not exist'.format(requirements_file) + assert error_msg in str(excinfo.value) def test_python_install_requirements_does_not_allow_null(self, tmpdir): build = self.get_build_config( @@ -1435,7 +1430,7 @@ def test_python_install_extra_requirements_and_setuptools(self, tmpdir): build.validate() assert excinfo.value.key == 'python.install.0.extra_requirements' - @pytest.mark.parametrize('value', [2, 'invalid', {}]) + @pytest.mark.parametrize('value', [2, 'invalid', {}, '', None]) def test_python_install_extra_requirements_check_type(self, value, tmpdir): build = self.get_build_config( { From 4159fedfd4f03dc98f08d03dd7fedcf7b70e5250 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 5 Dec 2018 13:45:17 -0500 Subject: [PATCH 25/28] Don't return --- readthedocs/doc_builder/python_environments.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index a1dab7d7de7..41944b29239 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -73,9 +73,9 @@ def install_requirements(self): """Install all requirements from the config object.""" for install in self.config.python.install: if isinstance(install, PythonInstallRequirements): - return self.install_requirements_file(install) + self.install_requirements_file(install) if isinstance(install, PythonInstall): - return self.install_package(install) + self.install_package(install) def install_package(self, install): """Install the package using pip or setuptools.""" From 808e83baf2921760e3d41b3c2e1464d156d6713b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 5 Dec 2018 13:59:49 -0500 Subject: [PATCH 26/28] Don't touch original config --- readthedocs/config/config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 86d34f65d91..d3638d6c1d8 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -5,13 +5,14 @@ """Build configuration for rtd.""" from __future__ import division, print_function, unicode_literals +import copy import os import re from contextlib import contextmanager import six -from readthedocs.config.utils import to_dict, list_to_dict +from readthedocs.config.utils import list_to_dict, to_dict from readthedocs.projects.constants import DOCUMENTATION_CHOICES from .find import find_one @@ -162,7 +163,7 @@ class BuildConfigBase(object): def __init__(self, env_config, raw_config, source_file): self.env_config = env_config - self.raw_config = raw_config + self.raw_config = copy.deepcopy(raw_config) self.source_file = source_file if os.path.isdir(self.source_file): self.base_path = self.source_file From 0df97dc1a09ad210fe2f74fae4dc78b51ae69a43 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Dec 2018 11:41:18 -0500 Subject: [PATCH 27/28] Better docstrings and comments --- readthedocs/doc_builder/python_environments.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index 41944b29239..f337e872da4 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -78,7 +78,12 @@ def install_requirements(self): self.install_package(install) def install_package(self, install): - """Install the package using pip or setuptools.""" + """ + Install the package using pip or setuptools. + + :param install: A install object from the config module. + :type install: readthedocs.config.models.PythonInstall + """ rel_path = os.path.relpath(install.path, self.checkout_path) if install.method == PIP: # Prefix ./ so pip installs from a local path rather than pypi @@ -310,11 +315,16 @@ def install_core_requirements(self): ) def install_requirements_file(self, install): - """Install a requirements file using pip.""" + """ + Install a requirements file using pip. + + :param install: A install object from the config module. + :type install: readthedocs.config.models.PythonInstallRequirements + """ requirements_file_path = install.requirements - # This only happens when the config file is from v1. - # We try to find a requirements file. if requirements_file_path is None: + # This only happens when the config file is from v1. + # We try to find a requirements file. builder_class = get_builder_class(self.config.doctype) docs_dir = (builder_class(build_env=self.build_env, python_env=self) .docs_dir()) From 0345845d0089dbc0f364461f6d0da8f0e9ae6c28 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Jan 2019 12:16:13 -0500 Subject: [PATCH 28/28] Fix merge --- readthedocs/config/config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 403b2c9bf14..71aa45b2469 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -32,6 +32,7 @@ validate_bool, validate_choice, validate_dict, + validate_directory, validate_file, validate_list, validate_string,