From 9275a560a43a1f3ac3e86b72b00a0802d07b5aaa Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 24 Jul 2023 11:51:55 +0200 Subject: [PATCH 1/8] Docs: do not mention `python.system_packages` This option doesn't work on builds using `build.os` because the Python binary used it's not the one from the system, but another one compiled via `asdf`. This means that creating a virtualenv with access to the "system packages" has no effect at all. As a first step, I'm removing this option from the documentation to avoid confusions. Next, we should probably our deprecation policy to contact users ands remove it from the code as well. Closes #10500 --- docs/user/config-file/v2.rst | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/docs/user/config-file/v2.rst b/docs/user/config-file/v2.rst index 6984e9298da..607461bf676 100644 --- a/docs/user/config-file/v2.rst +++ b/docs/user/config-file/v2.rst @@ -130,7 +130,6 @@ Configuration of the Python environment to be used. - docs - method: pip path: another/package - system_packages: true python.version `````````````` @@ -228,20 +227,6 @@ With the previous settings, Read the Docs will execute the next commands: pip install .[docs] -python.system_packages -`````````````````````` - -Give the virtual environment access to the global site-packages directory. - -:Type: ``bool`` -:Default: ``false`` - -.. warning:: - - If you are using a :ref:`Conda ` environment - to manage the build, this setting will not have any effect, since - the virtual environment creation is managed by Conda. - conda ~~~~~ @@ -868,8 +853,6 @@ Changes - The settings ``python.setup_py_install`` and ``python.pip_install`` were replaced by ``python.install``. And now it accepts a path to the package. See :ref:`config-file/v2:Packages`. -- The setting ``python.use_system_site_packages`` was renamed to ``python.system_packages``. - See :ref:`config-file/v2:python.system_packages`. - The build will fail if there are invalid keys (strict mode). .. warning:: From c5135687d9f5ceb262f7e93c9de3972bca155419 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Aug 2023 14:36:09 +0200 Subject: [PATCH 2/8] Build: remove "use system package" option - Removed from the UI - Removed from the v1 and v2 config file --- readthedocs/api/v2/serializers.py | 1 - readthedocs/config/config.py | 19 ------ readthedocs/config/tests/test_config.py | 58 ------------------- readthedocs/doc_builder/config.py | 1 - readthedocs/projects/fixtures/test_data.json | 17 ------ readthedocs/projects/forms.py | 1 - readthedocs/projects/models.py | 8 --- .../projects/tests/test_build_tasks.py | 56 ------------------ .../rtd_tests/fixtures/spec/v2/schema.json | 6 -- .../rtd_tests/fixtures/spec/v2/schema.yml | 4 -- readthedocs/rtd_tests/tests/test_api.py | 1 - .../rtd_tests/tests/test_build_config.py | 24 -------- .../tests/test_config_integration.py | 1 - .../rtd_tests/tests/test_doc_building.py | 41 ------------- 14 files changed, 238 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 521a2e7acf4..e9266ecee6e 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -83,7 +83,6 @@ class Meta(ProjectSerializer.Meta): "container_mem_limit", "container_time_limit", "install_project", - "use_system_packages", "skip", "requirements_file", "python_interpreter", diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 7471d9f1a94..7a57a137aa8 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -506,10 +506,8 @@ def validate_build(self): def validate_python(self): """Validates the ``python`` key, set default values it's necessary.""" install_project = self.defaults.get('install_project', False) - use_system_packages = self.defaults.get('use_system_packages', False) version = self.defaults.get('python_version', '2') python = { - 'use_system_site_packages': use_system_packages, 'install_with_pip': False, 'extra_requirements': [], 'install_with_setup': install_project, @@ -525,13 +523,6 @@ def validate_python(self): code=PYTHON_INVALID, ) - # Validate use_system_site_packages. - if 'use_system_site_packages' in raw_python: - with self.catch_validation_error('python.use_system_site_packages'): - python['use_system_site_packages'] = validate_bool( - raw_python['use_system_site_packages'], - ) - # Validate pip_install. if 'pip_install' in raw_python: with self.catch_validation_error('python.pip_install'): @@ -662,7 +653,6 @@ def python(self): return Python( version=python['version'], install=python_install, - use_system_site_packages=python['use_system_site_packages'], ) @property @@ -966,7 +956,6 @@ def validate_python(self): Fall back to the defaults of: - ``requirements`` - ``install`` (only for setup.py method) - - ``system_packages`` .. note:: - ``version`` can be a string or number type. @@ -1010,13 +999,6 @@ def validate_python(self): for index in range(len(raw_install)) ] - with self.catch_validation_error('python.system_packages'): - system_packages = self.pop_config( - 'python.system_packages', - False, - ) - python['use_system_site_packages'] = validate_bool(system_packages) - return python def validate_python_install(self, index): @@ -1353,7 +1335,6 @@ def python(self): return Python( version=python.get('version'), install=python_install, - use_system_site_packages=python['use_system_site_packages'], ) @property diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 5f447c3e377..33441435bf9 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -312,17 +312,6 @@ def test_use_system_site_packages_defaults_to_false(): assert not build.python.use_system_site_packages -@pytest.mark.parametrize('value', [True, False]) -def test_use_system_site_packages_repects_default_value(value): - defaults = { - 'use_system_packages': value, - } - build = get_build_config({}, {'defaults': defaults}) - build.validate() - assert build.python.use_system_site_packages is value - - - class TestValidatePythonExtraRequirements: def test_it_defaults_to_install_requirements_as_none(self): @@ -1801,53 +1790,6 @@ def test_python_install_several_respects_order(self, tmpdir): assert install[2].requirements == 'three.txt' - @pytest.mark.parametrize('value', [True, False]) - def test_python_system_packages_check_valid(self, value): - build = self.get_build_config({ - 'python': { - 'system_packages': value, - }, - }) - build.validate() - assert build.python.use_system_site_packages is value - - @pytest.mark.parametrize('value', [[], 'invalid', 5]) - def test_python_system_packages_check_invalid(self, value): - build = self.get_build_config({ - 'python': { - 'system_packages': value, - }, - }) - with raises(InvalidConfig) as excinfo: - build.validate() - assert excinfo.value.key == 'python.system_packages' - - def test_python_system_packages_check_default(self): - build = self.get_build_config({}) - build.validate() - assert build.python.use_system_site_packages is False - - def test_python_system_packages_dont_respects_default(self): - build = self.get_build_config( - {}, - {'defaults': {'use_system_packages': True}}, - ) - build.validate() - assert build.python.use_system_site_packages is False - - def test_python_system_packages_priority_over_default(self): - build = self.get_build_config( - {'python': {'system_packages': False}}, - ) - build.validate() - assert build.python.use_system_site_packages is False - - build = self.get_build_config( - {'python': {'system_packages': True}}, - ) - build.validate() - assert build.python.use_system_site_packages is True - @pytest.mark.parametrize('value', [[], True, 0, 'invalid']) def test_sphinx_validate_type(self, value): build = self.get_build_config({'sphinx': value}) diff --git a/readthedocs/doc_builder/config.py b/readthedocs/doc_builder/config.py index 7e048057b22..be554203829 100644 --- a/readthedocs/doc_builder/config.py +++ b/readthedocs/doc_builder/config.py @@ -44,7 +44,6 @@ def load_yaml_config(version, readthedocs_yaml_path=None): 'defaults': { 'install_project': project.install_project, 'formats': get_default_formats(project), - 'use_system_packages': project.use_system_packages, 'requirements_file': project.requirements_file, 'python_version': python_version, 'sphinx_configuration': sphinx_configuration, diff --git a/readthedocs/projects/fixtures/test_data.json b/readthedocs/projects/fixtures/test_data.json index bee4451f6a5..b87506c1b24 100644 --- a/readthedocs/projects/fixtures/test_data.json +++ b/readthedocs/projects/fixtures/test_data.json @@ -40,7 +40,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -94,7 +93,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -148,7 +146,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -202,7 +199,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -256,7 +252,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -310,7 +305,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -364,7 +358,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -418,7 +411,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -472,7 +464,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -526,7 +517,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -580,7 +570,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -634,7 +623,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -688,7 +676,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -742,7 +729,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -794,7 +780,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -846,7 +831,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", @@ -898,7 +882,6 @@ "skip": false, "install_project": false, "python_interpreter": "python3", - "use_system_packages": false, "privacy_level": "public", "language": "en", "programming_language": "words", diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 724f85210df..b034ea6ff6e 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -225,7 +225,6 @@ class Meta: 'requirements_file', 'python_interpreter', 'install_project', - 'use_system_packages', 'conf_py_file', 'enable_pdf_build', 'enable_epub_build', diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 834f55fa4b5..bdef810aaf8 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -454,14 +454,6 @@ class Project(models.Model): ), ) - use_system_packages = models.BooleanField( - _('Use system packages'), - help_text=_( - 'Give the virtual environment access to the global ' - 'site-packages dir.', - ), - default=False, - ) privacy_level = models.CharField( _('Privacy Level'), max_length=20, diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 4e340a4cfee..1cc224da9f0 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -1395,62 +1395,6 @@ def test_mkdocs_fail_on_warning(self, load_yaml_config): ] ) - @mock.patch("readthedocs.doc_builder.director.load_yaml_config") - def test_system_site_packages(self, load_yaml_config): - load_yaml_config.return_value = self._config_file( - { - "version": 2, - "python": { - "system_packages": True, - }, - }, - ) - - self._trigger_update_docs_task() - - self.mocker.mocks["environment.run"].assert_has_calls( - [ - mock.call( - "python3.7", - "-mvirtualenv", - "--system-site-packages", # expected flag - mock.ANY, - bin_path=None, - cwd=None, - ), - ] - ) - - @mock.patch("readthedocs.doc_builder.director.load_yaml_config") - def test_system_site_packages_project_overrides(self, load_yaml_config): - load_yaml_config.return_value = self._config_file( - { - "version": 2, - # Do not define `system_packages: True` in the config file. - "python": {}, - }, - ) - - # Override the setting in the Project object - self.project.use_system_packages = True - self.project.save() - - self._trigger_update_docs_task() - - self.mocker.mocks["environment.run"].assert_has_calls( - [ - mock.call( - "python3.7", - "-mvirtualenv", - # we don't expect this flag to be here - # '--system-site-packages' - mock.ANY, - bin_path=None, - cwd=None, - ), - ] - ) - @mock.patch("readthedocs.doc_builder.director.load_yaml_config") def test_python_install_setuptools(self, load_yaml_config): load_yaml_config.return_value = self._config_file( diff --git a/readthedocs/rtd_tests/fixtures/spec/v2/schema.json b/readthedocs/rtd_tests/fixtures/spec/v2/schema.json index 438a050753d..6ff1d69b547 100644 --- a/readthedocs/rtd_tests/fixtures/spec/v2/schema.json +++ b/readthedocs/rtd_tests/fixtures/spec/v2/schema.json @@ -260,12 +260,6 @@ "default": "3", "deprecated": true }, - "system_packages": { - "title": "System Packages", - "description": "Give the virtual environment access to the global site-packages directory.", - "type": "boolean", - "default": false - }, "install": { "title": "Install", "description": "Installation of packages and requiremens.", diff --git a/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml b/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml index 54d2a1aaee3..0a21a600582 100644 --- a/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml +++ b/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml @@ -59,10 +59,6 @@ python: # Default: '3' version: enum('2', '2.7', '3', '3.3', '3.4', '3.5', '3.6', required=False) - # Give the virtual environment access to the global site-packages dir - # Default: false | project config - system_packages: bool(required=False) - # Installation of packages and requiremens install: list(any(include('python-install-requirements'), include('python-install')), required=False) diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index f0da2e55767..5fb2390f216 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -3130,7 +3130,6 @@ def test_get_version_by_id(self): "show_advertising": True, "skip": False, "slug": "pip", - "use_system_packages": False, "users": [1], "urlconf": None, }, diff --git a/readthedocs/rtd_tests/tests/test_build_config.py b/readthedocs/rtd_tests/tests/test_build_config.py index b501be50e39..7c093153a72 100644 --- a/readthedocs/rtd_tests/tests/test_build_config.py +++ b/readthedocs/rtd_tests/tests/test_build_config.py @@ -327,30 +327,6 @@ def test_python_install_extra_requirements_empty(tmpdir, value): assertValidConfig(tmpdir, content.format(value=value)) -@pytest.mark.parametrize('value', ['true', 'false']) -def test_python_system_packages(tmpdir, value): - content = ''' -version: "2" -python: - system_packages: {value} - ''' - assertValidConfig(tmpdir, content.format(value=value)) - - -@pytest.mark.parametrize('value', ['not true', "''", '[]']) -def test_python_system_packages_invalid(tmpdir, value): - content = ''' -version: "2" -python: - system_packages: {value} - ''' - assertInvalidConfig( - tmpdir, - content.format(value=value), - ['is not a bool'], - ) - - def test_sphinx(tmpdir): content = ''' version: "2" diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index 33933aced1b..3f5ee432fd6 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -83,7 +83,6 @@ def test_python_supported_versions_default_image_1_0(self, load_config): 'epub', 'pdf' ], - 'use_system_packages': self.project.use_system_packages, 'requirements_file': self.project.requirements_file, 'python_version': '3', 'sphinx_configuration': mock.ANY, diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 6583cd41679..f41c88a041b 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -11,7 +11,6 @@ from docker.errors import APIError as DockerAPIError from readthedocs.builds.models import Version -from readthedocs.doc_builder.config import load_yaml_config from readthedocs.doc_builder.environments import ( BuildCommand, DockerBuildCommand, @@ -22,7 +21,6 @@ from readthedocs.doc_builder.python_environments import Conda, Virtualenv from readthedocs.projects.models import Project from readthedocs.rtd_tests.mocks.paths import fake_paths_lookup -from readthedocs.rtd_tests.tests.test_config_integration import create_load DUMMY_BUILD_ID = 123 SAMPLE_UNICODE = 'HérÉ îß sömê ünïçó∂é' @@ -427,45 +425,6 @@ def test_install_core_requirements_sphinx(self, checkout_path): args = self.pip_install_args + requirements self.assertArgsStartsWith(args, calls[1]) - @mock.patch('readthedocs.doc_builder.config.load_config') - @patch('readthedocs.projects.models.Project.checkout_path') - def test_install_core_requirements_sphinx_system_packages_caps_setuptools(self, checkout_path, load_config): - config_data = { - 'python': { - 'use_system_site_packages': True, - }, - } - load_config.side_effect = create_load(config_data) - config = load_yaml_config(self.version_sphinx) - - tmpdir = tempfile.mkdtemp() - checkout_path.return_value = tmpdir - python_env = Virtualenv( - version=self.version_sphinx, - build_env=self.build_env_mock, - config=config, - ) - python_env.install_core_requirements() - requirements_sphinx = [ - "commonmark", - "recommonmark", - "sphinx", - "sphinx-rtd-theme", - "readthedocs-sphinx-ext", - "jinja2<3.1.0", - "setuptools", - ] - - self.assertEqual(self.build_env_mock.run.call_count, 2) - calls = self.build_env_mock.run.call_args_list - - core_args = self.pip_install_args + ["pip", "setuptools"] - self.assertArgsStartsWith(core_args, calls[0]) - - requirements = self.base_requirements + requirements_sphinx - args = self.pip_install_args + ['-I'] + requirements - self.assertArgsStartsWith(args, calls[1]) - @patch('readthedocs.projects.models.Project.checkout_path') def test_install_core_requirements_mkdocs(self, checkout_path): tmpdir = tempfile.mkdtemp() From 6b0230c42cb94d98ce7d004a18f5369ed17406fd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Aug 2023 15:34:07 +0200 Subject: [PATCH 3/8] Do not remove the DB field for now We should remove this field after the deploy. --- readthedocs/projects/models.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index bdef810aaf8..e00bb046010 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -454,6 +454,15 @@ class Project(models.Model): ), ) + # TODO: remove `use_system_packages` after deploying. + # This field is not used anymore. + use_system_packages = models.BooleanField( + _("Use system packages"), + help_text=_( + "Give the virtual environment access to the global site-packages dir.", + ), + default=False, + ) privacy_level = models.CharField( _('Privacy Level'), max_length=20, From 75bcc329939083523c6769123760f3d249285bdc Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Aug 2023 15:39:15 +0200 Subject: [PATCH 4/8] Remove `use_system_site_packages` leftovers --- docs/user/api/v3.rst | 1 - docs/user/config-file/v1.rst | 17 --------- readthedocs/config/models.py | 3 +- readthedocs/config/tests/test_config.py | 36 ------------------- .../doc_builder/python_environments.py | 15 ++------ .../projects/tests/test_build_tasks.py | 1 - 6 files changed, 3 insertions(+), 70 deletions(-) diff --git a/docs/user/api/v3.rst b/docs/user/api/v3.rst index 54b4f58fc7b..e1a04c49aa2 100644 --- a/docs/user/api/v3.rst +++ b/docs/user/api/v3.rst @@ -697,7 +697,6 @@ Build details "requirements": ".../stable/tools/docs-requirements.txt" } ], - "use_system_site_packages": false }, "conda": null, "build": { diff --git a/docs/user/config-file/v1.rst b/docs/user/config-file/v1.rst index ce54ca4738d..740f785c0da 100644 --- a/docs/user/config-file/v1.rst +++ b/docs/user/config-file/v1.rst @@ -167,23 +167,6 @@ the highest supported minor version will be selected. python: version: 3.5 -python.use_system_site_packages -``````````````````````````````` - -* Default: ``false`` -* Type: Boolean - -When true, it gives the virtual environment access to the global site-packages directory. -Depending on the :ref:`config-file/v1:build.image`, -Read the Docs includes some libraries like scipy, numpy, etc. -See :doc:`/builds` for more details. - -.. code-block:: yaml - - python: - use_system_site_packages: true - - python.setup_py_install ``````````````````````` diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index 785f6560277..cbb27b854a8 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -80,8 +80,7 @@ def __init__(self, **kwargs): class Python(Base): - - __slots__ = ('version', 'install', 'use_system_site_packages') + __slots__ = ("version", "install") class PythonInstallRequirements(Base): diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 33441435bf9..c238e842de2 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -305,13 +305,6 @@ def test_python_section_must_be_dict(): assert excinfo.value.code == PYTHON_INVALID -def test_use_system_site_packages_defaults_to_false(): - build = get_build_config({'python': {}}) - build.validate() - # Default is False. - assert not build.python.use_system_site_packages - - class TestValidatePythonExtraRequirements: def test_it_defaults_to_install_requirements_as_none(self): @@ -346,32 +339,6 @@ def test_it_uses_validate_string(self, validate_string): validate_string.assert_any_call('tests') -class TestValidateUseSystemSitePackages: - - def test_it_defaults_to_false(self): - build = get_build_config({'python': {}}) - build.validate() - assert build.python.use_system_site_packages is False - - def test_it_validates_value(self): - build = get_build_config( - {'python': {'use_system_site_packages': 'invalid'}}, - ) - with raises(InvalidConfig) as excinfo: - build.validate() - excinfo.value.key = 'python.use_system_site_packages' - excinfo.value.code = INVALID_BOOL - - @patch('readthedocs.config.config.validate_bool') - def test_it_uses_validate_bool(self, validate_bool): - validate_bool.return_value = True - build = get_build_config( - {'python': {'use_system_site_packages': 'to-validate'}}, - ) - build.validate() - validate_bool.assert_any_call('to-validate') - - class TestValidateSetupPyInstall: def test_it_defaults_to_false(self): @@ -785,7 +752,6 @@ def test_as_dict(tmpdir): 'install': [{ 'requirements': 'requirements.txt', }], - 'use_system_site_packages': False, }, 'build': { 'image': 'readthedocs/build:latest', @@ -2388,7 +2354,6 @@ def test_as_dict(self, tmpdir): 'install': [{ 'requirements': 'requirements.txt', }], - 'use_system_site_packages': False, }, 'build': { 'image': 'readthedocs/build:latest', @@ -2448,7 +2413,6 @@ def test_as_dict_new_build_config(self, tmpdir): 'install': [{ 'requirements': 'requirements.txt', }], - 'use_system_site_packages': False, }, 'build': { 'os': 'ubuntu-20.04', diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index 01127dd4090..d6091dba87a 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -136,12 +136,9 @@ def setup_base(self): """ cli_args = [ '-mvirtualenv', + # Append the positional destination argument + "$READTHEDOCS_VIRTUALENV_PATH", ] - if self.config.python.use_system_site_packages: - cli_args.append('--system-site-packages') - - # Append the positional destination argument - cli_args.append("$READTHEDOCS_VIRTUALENV_PATH") self.build_env.run( self.config.python_interpreter, @@ -292,14 +289,6 @@ def _install_old_requirements(self, pip_install_cmd): requirements.extend(["jinja2<3.1.0"]) cmd = copy.copy(pip_install_cmd) - if self.config.python.use_system_site_packages: - # Other code expects sphinx-build to be installed inside the - # virtualenv. Using the -I option makes sure it gets installed - # even if it is already installed system-wide (and - # --system-site-packages is used) - cmd.append('-I') - # The same version of setuptools used above needs to be used here. - requirements.append(setuptools_version) cmd.extend(requirements) self.build_env.run( *cmd, diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index b152db545be..af1ad085f7f 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -516,7 +516,6 @@ def test_successful_build( "python": { "version": "3", "install": [], - "use_system_site_packages": False, }, "conda": None, "build": { From 4561971b299338da49047d23f05474f630b39eca Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Aug 2023 18:56:21 +0200 Subject: [PATCH 5/8] Display custom error messages based on the config key --- readthedocs/config/config.py | 53 +++++++++++++------ readthedocs/config/tests/test_config.py | 7 ++- .../templates/builds/build_detail.html | 8 ++- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 161a28fd28e..d1aa2093ba0 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -2,6 +2,7 @@ """Build configuration for rtd.""" +import collections import copy import os import re @@ -128,14 +129,43 @@ class InvalidConfig(ConfigError): """Error for a specific key validation.""" - message_template = 'Invalid "{key}": {error}' + # Define the default message to show on ``InvalidConfig`` + default_message_template = 'Invalid configuration option "{key}"' + + # Create customized message for based on each particular ``key`` + message_templates = collections.defaultdict(lambda: "{default_message}: {error}") + + # Redirect the user to the blog post when using + # `python.system_packages` or `python.use_system_site_packages` + message_templates.update( + { + "python.system_packages": "{default_message}. " + "This was an old config key that's not supported anymore. " + "Please, refer to https://blog.readthedocs.com/use-system-packages-deprecated/ to read more about it and how to upgrade your config file." # noqa + } + ) + message_templates.update( + { + "python.use_system_site_packages": message_templates.get( + "python.system_packages" + ) + } + ) def __init__(self, key, code, error_message, source_file=None): self.key = key self.code = code self.source_file = source_file - message = self.message_template.format( - key=self._get_display_key(), + + display_key = self._get_display_key() + default_message = self.default_message_template.format( + key=display_key, + code=code, + error=error_message, + ) + message = self.message_templates[display_key].format( + default_message=default_message, + key=display_key, code=code, error=error_message, ) @@ -211,18 +241,10 @@ def __init__(self, env_config, raw_config, source_file, base_path=None): def error(self, key, message, code): """Raise an error related to ``key``.""" - if not os.path.isdir(self.source_file): - source = os.path.relpath(self.source_file, self.base_path) - error_message = '{source}: {message}'.format( - source=source, - message=message, - ) - else: - error_message = message raise InvalidConfig( key=key, code=code, - error_message=error_message, + error_message=message, source_file=self.source_file, ) @@ -1257,10 +1279,7 @@ def validate_keys(self): This should be called after all the validations are done and all keys are popped from `self._raw_config`. """ - msg = ( - 'Invalid configuration option: {}. ' - 'Make sure the key name is correct.' - ) + msg = "Make sure the key name is correct." # The version key isn't popped, but it's # validated in `load`. self.pop_config('version', None) @@ -1268,7 +1287,7 @@ def validate_keys(self): if wrong_key: self.error( wrong_key, - msg.format(wrong_key), + msg, code=INVALID_KEY, ) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index c238e842de2..a836e78ffad 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -802,7 +802,7 @@ def test_correct_error_when_source_is_dir(self, tmpdir): build.error(key='key', message='Message', code='code') # We don't have any extra information about # the source_file. - assert str(excinfo.value) == 'Invalid "key": Message' + assert str(excinfo.value) == 'Invalid configuration option "key": Message' def test_formats_check_valid(self): build = self.get_build_config({'formats': ['htmlzip', 'pdf', 'epub']}) @@ -1471,7 +1471,10 @@ def test_python_install_requirements_error_msg(self, tmpdir): with raises(InvalidConfig) as excinfo: build.validate() - assert str(excinfo.value) == 'Invalid "python.install[0].requirements": expected string' + assert ( + str(excinfo.value) + == 'Invalid configuration option "python.install[0].requirements": expected string' + ) def test_python_install_requirements_does_not_allow_empty_string(self, tmpdir): build = self.get_build_config( diff --git a/readthedocs/templates/builds/build_detail.html b/readthedocs/templates/builds/build_detail.html index 17bf9424074..64dc0c81805 100644 --- a/readthedocs/templates/builds/build_detail.html +++ b/readthedocs/templates/builds/build_detail.html @@ -227,7 +227,13 @@

{% trans "Build Errors" %}

{% trans "Error" %}

- {{ build.error }} + {# + I'd like to use ``build.error|urlize`` here, so we can have nice links. + However, this is not possible because we are using `data-bind="text: error"` + which means that Knockout.js will use the `.error` attribute to fill + the content of this tag dynamically. + #} + {{ build.error|urlize }}

{% block github_issue_link %} From 26d0748be5aef62f753819cd75f0b13e921a6f17 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 7 Aug 2023 12:43:03 +0200 Subject: [PATCH 6/8] Apply suggestions from code review Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com> --- readthedocs/config/config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index d1aa2093ba0..98ca01eb63a 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -140,10 +140,11 @@ class InvalidConfig(ConfigError): message_templates.update( { "python.system_packages": "{default_message}. " - "This was an old config key that's not supported anymore. " + "This configuration key has been deprecated and removed. " "Please, refer to https://blog.readthedocs.com/use-system-packages-deprecated/ to read more about it and how to upgrade your config file." # noqa } ) + # Use same message for `python.use_system_site_packages` message_templates.update( { "python.use_system_site_packages": message_templates.get( From cebb79e69faf0e1b858460076bff52c191c91c5b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 7 Aug 2023 12:45:06 +0200 Subject: [PATCH 7/8] Minor code style changes --- readthedocs/config/config.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 98ca01eb63a..50a16779e99 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -1280,15 +1280,14 @@ def validate_keys(self): This should be called after all the validations are done and all keys are popped from `self._raw_config`. """ - msg = "Make sure the key name is correct." # The version key isn't popped, but it's # validated in `load`. self.pop_config('version', None) wrong_key = '.'.join(self._get_extra_key(self._raw_config)) if wrong_key: self.error( - wrong_key, - msg, + key=wrong_key, + message="Make sure the key name is correct.", code=INVALID_KEY, ) From 71904ad3f4839285528c76f2fb4a80c2671da81e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 9 Aug 2023 22:50:46 +0200 Subject: [PATCH 8/8] Update readthedocs/config/config.py Co-authored-by: Anthony --- readthedocs/config/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 50a16779e99..27d4eba0886 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -141,7 +141,7 @@ class InvalidConfig(ConfigError): { "python.system_packages": "{default_message}. " "This configuration key has been deprecated and removed. " - "Please, refer to https://blog.readthedocs.com/use-system-packages-deprecated/ to read more about it and how to upgrade your config file." # noqa + "Refer to https://blog.readthedocs.com/use-system-packages-deprecated/ to read more about this change and how to upgrade your config file." # noqa } ) # Use same message for `python.use_system_site_packages`