From 0288239d2fe98f73f6c166badefeeb7537036fb4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 18 Jan 2023 12:59:39 +0100 Subject: [PATCH 01/10] Build: use environment variable to define output directory `READTHEDOCS_OUTPUT` points to the path where the user's repository was clonned plus `_readthedocs/` subdirectory. This way we can make the command nicer: ``` python -m sphinx -T -E -j auto -b singlehtml -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html ``` Also, we gives our users a clear contract to where to find those files, even if we change the path of the underlying variable in the future. --- readthedocs/doc_builder/backends/sphinx.py | 8 ++------ readthedocs/doc_builder/director.py | 3 +++ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index f0c65313860..e2a9f7e9c79 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -304,9 +304,7 @@ def build(self): # https://github.com/readthedocs/readthedocs.org/pull/9888#issuecomment-1384649346 ".", # Sphinx's output build directory (OUTPUTDIR) - os.path.relpath( - self.absolute_output_dir, os.path.dirname(self.config_file) - ), + os.path.join("$READTHEDOCS_OUTPUT", "html"), ] ) cmd_ret = self.run( @@ -531,9 +529,7 @@ def build(self): # https://github.com/readthedocs/readthedocs.org/pull/9888#issuecomment-1384649346 ".", # Sphinx's output build directory (OUTPUTDIR) - os.path.relpath( - self.absolute_output_dir, os.path.dirname(self.config_file) - ), + os.path.join("$READTHEDOCS_OUTPUT", "pdf"), cwd=os.path.dirname(self.config_file), bin_path=self.python_env.venv_bin(), ) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index fedd6ed7f63..d0de50ea59d 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -543,6 +543,9 @@ def get_rtd_env_vars(self): "READTHEDOCS_VERSION_NAME": self.data.version.verbose_name, "READTHEDOCS_PROJECT": self.data.project.slug, "READTHEDOCS_LANGUAGE": self.data.project.language, + "READTHEDOCS_OUTPUT": os.path.join( + self.data.project.checkout_path(self.data.version.slug), "_readthedocs/" + ), } return env From c741afbc2d922e67a774e3388f8093f21014b9da Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Wed, 18 Jan 2023 15:25:24 -0800 Subject: [PATCH 02/10] Log environment during build commands --- readthedocs/doc_builder/environments.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 979fc18dda8..71d2cee2676 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -130,8 +130,6 @@ def __str__(self): # commands, which is not supported anymore def run(self): """Set up subprocess and execute command.""" - log.info("Running build command.", command=self.get_command(), cwd=self.cwd) - self.start_time = datetime.utcnow() environment = self._environment.copy() if 'DJANGO_SETTINGS_MODULE' in environment: @@ -145,6 +143,13 @@ def run(self): env_paths.insert(0, self.bin_path) environment['PATH'] = ':'.join(env_paths) + log.info( + "Running build command.", + command=self.get_command(), + cwd=self.cwd, + environment=environment, + ) + try: # When using ``shell=True`` the command should be flatten command = self.command From 382d332856f6a68eaec9323ca4c17f5aa69ea939 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Wed, 18 Jan 2023 15:54:24 -0800 Subject: [PATCH 03/10] Love the environment in the container build as well --- readthedocs/doc_builder/environments.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 71d2cee2676..7befc4cffe1 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -297,6 +297,7 @@ def run(self): container_id=self.build_env.container_id, command=self.get_command(), cwd=self.cwd, + environment=self._environment, ) self.start_time = datetime.utcnow() From 300d30342b9d5272442144afb16b7e79e3f00ff2 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 31 Jan 2023 12:06:47 +0100 Subject: [PATCH 04/10] Build: do not escape `$READTHEDOCS_OUTPUT` variable Add a simple hack to avoid escaping this internal variable since we need it to determine the output path. --- readthedocs/doc_builder/environments.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 7befc4cffe1..7ba3daf84f8 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -386,7 +386,11 @@ def get_wrapped_command(self): def _escape_command(self, cmd): r"""Escape the command by prefixing suspicious chars with `\`.""" - return self.bash_escape_re.sub(r'\\\1', cmd) + command = self.bash_escape_re.sub(r"\\\1", cmd) + + # HACK: avoid escaping `$READTHEDOCS_OUTPUT` variable + command = command.replace("\\$READTHEDOCS_OUTPUT", "$READTHEDOCS_OUTPUT") + return command class BaseEnvironment: From bd9161e3516cc3e471df3c589d37e071b6673c93 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 31 Jan 2023 12:10:44 +0100 Subject: [PATCH 05/10] Build: use `$READTHEDOCS_OUTPUT` variable for MkDocs builder --- readthedocs/doc_builder/backends/mkdocs.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/readthedocs/doc_builder/backends/mkdocs.py b/readthedocs/doc_builder/backends/mkdocs.py index 4d6ae67153b..0d0174176d0 100644 --- a/readthedocs/doc_builder/backends/mkdocs.py +++ b/readthedocs/doc_builder/backends/mkdocs.py @@ -296,17 +296,8 @@ def build(self): 'mkdocs', self.builder, "--clean", - # ``site_dir`` is relative to where the mkdocs.yaml file is - # https://www.mkdocs.org/user-guide/configuration/#site_dir - # Example: - # - # when ``--config-file=docs/mkdocs.yml``, - # it must be ``--site-dir=../_readthedocs/html`` "--site-dir", - os.path.join( - os.path.relpath(self.project_path, os.path.dirname(self.yaml_file)), - self.build_dir, - ), + os.path.join("$READTHEDOCS_OUTPUT", "html"), "--config-file", os.path.relpath(self.yaml_file, self.project_path), ] From 90716a1f85dcd18b6adb77fdd17fd0b2f37d623d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 1 Feb 2023 14:08:43 +0100 Subject: [PATCH 06/10] Log: do not log the environment It could contain some private data. --- readthedocs/doc_builder/environments.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 7ba3daf84f8..165d9b54f2f 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -297,7 +297,6 @@ def run(self): container_id=self.build_env.container_id, command=self.get_command(), cwd=self.cwd, - environment=self._environment, ) self.start_time = datetime.utcnow() From b183d23f473ed17ddd7839ea92bd47a52069f2c6 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 7 Feb 2023 12:00:59 +0100 Subject: [PATCH 07/10] Build: organize absolute output directory for host and container There are some commands that are executed from inside the container where `$READTHEDOCS_OUTPUT` variable is defined and we can use it. However, there are other commands executed from the host where that variable is not defined and/or it cannot be used (e.g. as a `cwd=` argument for Docker run command). --- readthedocs/doc_builder/backends/sphinx.py | 88 +++++++++++++--------- 1 file changed, 53 insertions(+), 35 deletions(-) diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index e2a9f7e9c79..d86946070f8 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -42,15 +42,15 @@ class BaseSphinx(BaseBuilder): # an output file, the parsed source files are cached as "doctree pickles". sphinx_doctrees_dir = "_build/doctrees" - # Output directory relative to where the repository was cloned - # (e.g. "_readthedocs/") + # Output directory relative to $READTHEDOCS_OUTPUT + # (e.g. "html", "htmlzip" or "pdf") relative_output_dir = None def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.config_file = self.config.sphinx.configuration - self.absolute_output_dir = os.path.abspath( - os.path.join(self.project_path, self.relative_output_dir) + self.absolute_output_dir = os.path.join( + "$READTHEDOCS_OUTPUT", self.relative_output_dir ) try: if not self.config_file: @@ -304,7 +304,7 @@ def build(self): # https://github.com/readthedocs/readthedocs.org/pull/9888#issuecomment-1384649346 ".", # Sphinx's output build directory (OUTPUTDIR) - os.path.join("$READTHEDOCS_OUTPUT", "html"), + self.absolute_output_dir, ] ) cmd_ret = self.run( @@ -361,9 +361,22 @@ def venv_sphinx_supports_latexmk(self): ) return cmd_ret.exit_code == 0 + def _get_absolute_host_output_dir(self): + # NOTE: we cannot use `$READTHEDOCS_OUTPUT` environment variable here + # because it's not defined in the host. So, we have to re-calculate its + # value. We will remove this limitation when we execute the whole + # building from inside the Docker container (instead behing a hybrid as it is now) + readthedocs_output = os.path.join( + self.project.checkout_path(self.version.slug), "_readthedocs/" + ) + absolute_host_output_dir = os.path.join( + readthedocs_output, self.relative_output_dir + ) + return absolute_host_output_dir + class HtmlBuilder(BaseSphinx): - relative_output_dir = "_readthedocs/html" + relative_output_dir = "html" def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -392,18 +405,16 @@ def __init__(self, *args, **kwargs): class LocalMediaBuilder(BaseSphinx): sphinx_builder = 'readthedocssinglehtmllocalmedia' - relative_output_dir = "_readthedocs/htmlzip" + relative_output_dir = "htmlzip" def _post_build(self): """Internal post build to create the ZIP file from the HTML output.""" - target_file = os.path.abspath( - os.path.join( - self.absolute_output_dir, - # TODO: shouldn't this name include the name of the version as well? - # It seems we were using the project's slug previously. - # So, keeping it like that for now until we decide make that adjustment. - f"{self.project.slug}.zip", - ) + target_file = os.path.join( + self.absolute_output_dir, + # TODO: shouldn't this name include the name of the version as well? + # It seems we were using the project's slug previously. + # So, keeping it like that for now until we decide make that adjustment. + f"{self.project.slug}.zip", ) # **SECURITY CRITICAL: Advisory GHSA-hqwg-gjqw-h5wg** @@ -415,7 +426,7 @@ def _post_build(self): dirname = f"{self.project.slug}-{self.version.slug}" self.run( "mv", - self.relative_output_dir, + self.absolute_output_dir, str(tmp_dir / dirname), cwd=self.project_path, record=False, @@ -423,7 +434,7 @@ def _post_build(self): self.run( "mkdir", "--parents", - self.relative_output_dir, + self.absolute_output_dir, cwd=self.project_path, record=False, ) @@ -441,7 +452,7 @@ def _post_build(self): class EpubBuilder(BaseSphinx): sphinx_builder = "epub" - relative_output_dir = "_readthedocs/epub" + relative_output_dir = "epub" def _post_build(self): """Internal post build to cleanup EPUB output directory and leave only one .epub file.""" @@ -451,7 +462,9 @@ def _post_build(self): f"{self.project.slug}.epub", ) - epub_sphinx_filepaths = glob(os.path.join(self.absolute_output_dir, "*.epub")) + epub_sphinx_filepaths = glob( + os.path.join(self._get_absolute_host_output_dir(), "*.epub") + ) if epub_sphinx_filepaths: # NOTE: we currently support only one .epub per version epub_filepath = epub_sphinx_filepaths[0] @@ -462,14 +475,14 @@ def _post_build(self): self.run( "rm", "--recursive", - self.relative_output_dir, + self.absolute_output_dir, cwd=self.project_path, record=False, ) self.run( "mkdir", "--parents", - self.relative_output_dir, + self.absolute_output_dir, cwd=self.project_path, record=False, ) @@ -506,7 +519,7 @@ class PdfBuilder(BaseSphinx): """Builder to generate PDF documentation.""" - relative_output_dir = "_readthedocs/pdf" + relative_output_dir = "pdf" sphinx_builder = "latex" pdf_file_name = None @@ -529,12 +542,12 @@ def build(self): # https://github.com/readthedocs/readthedocs.org/pull/9888#issuecomment-1384649346 ".", # Sphinx's output build directory (OUTPUTDIR) - os.path.join("$READTHEDOCS_OUTPUT", "pdf"), + self.absolute_output_dir, cwd=os.path.dirname(self.config_file), bin_path=self.python_env.venv_bin(), ) - tex_files = glob(os.path.join(self.absolute_output_dir, "*.tex")) + tex_files = glob(os.path.join(self._get_absolute_host_output_dir(), "*.tex")) if not tex_files: raise BuildUserError("No TeX files were found.") @@ -554,7 +567,9 @@ def _build_latexmk(self, cwd): # https://github.com/sphinx-doc/sphinx/blob/master/sphinx/texinputs/Makefile_t images = [] for extension in ("png", "gif", "jpg", "jpeg"): - images.extend(Path(self.absolute_output_dir).glob(f"*.{extension}")) + images.extend( + Path(self._get_absolute_host_output_dir()).glob(f"*.{extension}") + ) # FIXME: instead of checking by language here, what we want to check if # ``latex_engine`` is ``platex`` @@ -564,13 +579,13 @@ def _build_latexmk(self, cwd): # step. I don't know exactly why but most of the documentation that # I read differentiate this language from the others. I suppose # it's because it mix kanji (Chinese) with its own symbols. - pdfs = Path(self.absolute_output_dir).glob("*.pdf") + pdfs = Path(self._get_absolute_host_output_dir()).glob("*.pdf") for image in itertools.chain(images, pdfs): self.run( 'extractbb', image.name, - cwd=self.absolute_output_dir, + cwd=self._get_absolute_host_output_dir(), record=False, ) @@ -581,7 +596,7 @@ def _build_latexmk(self, cwd): self.run( 'cat', rcfile, - cwd=self.absolute_output_dir, + cwd=self._get_absolute_host_output_dir(), ) if self.build_env.command_class == DockerBuildCommand: @@ -609,7 +624,7 @@ def _build_latexmk(self, cwd): cls=latex_class, cmd=cmd, warn_only=True, - cwd=self.absolute_output_dir, + cwd=self._get_absolute_host_output_dir(), ) self.pdf_file_name = f'{self.project.slug}.pdf' @@ -644,7 +659,7 @@ def _build_pdflatex(self, tex_files): cmd_ret = self.build_env.run_command_class( cls=latex_class, cmd=cmd, - cwd=self.absolute_output_dir, + cwd=self._get_absolute_host_output_dir(), warn_only=True, ) pdf_commands.append(cmd_ret) @@ -652,7 +667,7 @@ def _build_pdflatex(self, tex_files): cmd_ret = self.build_env.run_command_class( cls=latex_class, cmd=cmd, - cwd=self.absolute_output_dir, + cwd=self._get_absolute_host_output_dir(), warn_only=True, ) pdf_commands.append(cmd_ret) @@ -660,7 +675,7 @@ def _build_pdflatex(self, tex_files): cmd_ret = self.build_env.run_command_class( cls=latex_class, cmd=cmd, - cwd=self.absolute_output_dir, + cwd=self._get_absolute_host_output_dir(), warn_only=True, ) pdf_match = PDF_RE.search(cmd_ret.output) @@ -684,7 +699,10 @@ def _post_build(self): # NOTE: we currently support only one .pdf per version pdf_sphinx_filepath = os.path.join(self.absolute_output_dir, self.pdf_file_name) - if os.path.exists(pdf_sphinx_filepath): + pdf_sphinx_filepath_host = os.path.join( + self._get_absolute_host_output_dir(), self.pdf_file_name + ) + if os.path.exists(pdf_sphinx_filepath_host): self.run( "mv", pdf_sphinx_filepath, @@ -695,14 +713,14 @@ def _post_build(self): self.run( "rm", "-r", - self.relative_output_dir, + self.absolute_output_dir, cwd=self.project_path, record=False, ) self.run( "mkdir", "-p", - self.relative_output_dir, + self.absolute_output_dir, cwd=self.project_path, record=False, ) From 837ff3d1e8a2de57d964d7257c585f0cc473f50a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 7 Feb 2023 12:03:03 +0100 Subject: [PATCH 08/10] Test: update tests to use the new variable --- .../projects/tests/test_build_tasks.py | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index e1b2d28e973..89e4c9e461c 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -120,7 +120,7 @@ def test_build_sphinx_formats(self, load_yaml_config, formats, builders): "-D", "language=en", ".", - "../_readthedocs/html", + "$READTHEDOCS_OUTPUT/html", cwd=mock.ANY, bin_path=mock.ANY, ) @@ -141,7 +141,7 @@ def test_build_sphinx_formats(self, load_yaml_config, formats, builders): "-D", "language=en", ".", - "../_readthedocs/html", + "$READTHEDOCS_OUTPUT/html", cwd=mock.ANY, bin_path=mock.ANY, ) @@ -275,6 +275,9 @@ def test_get_env_vars(self, load_yaml_config, build_environment, config, externa "READTHEDOCS_VERSION_NAME": self.version.verbose_name, "READTHEDOCS_PROJECT": self.project.slug, "READTHEDOCS_LANGUAGE": self.project.language, + "READTHEDOCS_OUTPUT": os.path.join( + self.project.checkout_path(self.version.slug), "_readthedocs/" + ), } self._trigger_update_docs_task() @@ -650,7 +653,7 @@ def test_build_commands_executed( "-D", "language=en", ".", - "../_readthedocs/html", + "$READTHEDOCS_OUTPUT/html", cwd=mock.ANY, bin_path=mock.ANY, ), @@ -667,7 +670,7 @@ def test_build_commands_executed( "-D", "language=en", ".", - "../_readthedocs/htmlzip", + "$READTHEDOCS_OUTPUT/htmlzip", cwd=mock.ANY, bin_path=mock.ANY, ), @@ -712,7 +715,7 @@ def test_build_commands_executed( "-D", "language=en", ".", - "../_readthedocs/pdf", + "$READTHEDOCS_OUTPUT/pdf", cwd=mock.ANY, bin_path=mock.ANY, ), @@ -741,7 +744,7 @@ def test_build_commands_executed( "-D", "language=en", ".", - "../_readthedocs/epub", + "$READTHEDOCS_OUTPUT/epub", cwd=mock.ANY, bin_path=mock.ANY, ), @@ -753,12 +756,16 @@ def test_build_commands_executed( record=False, ), mock.call( - "rm", "--recursive", "_readthedocs/epub", cwd=mock.ANY, record=False + "rm", + "--recursive", + "$READTHEDOCS_OUTPUT/epub", + cwd=mock.ANY, + record=False, ), mock.call( "mkdir", "--parents", - "_readthedocs/epub", + "$READTHEDOCS_OUTPUT/epub", cwd=mock.ANY, record=False, ), @@ -1009,7 +1016,7 @@ def test_build_commands(self, load_yaml_config): }, "commands": [ "pip install pelican[markdown]", - "pelican --settings docs/pelicanconf.py --output _readthedocs/html/ docs/", + "pelican --settings docs/pelicanconf.py --output $READTHEDOCS_OUTPUT/html/ docs/", ], }, }, @@ -1054,7 +1061,7 @@ def test_build_commands(self, load_yaml_config): "--settings", "docs/pelicanconf.py", "--output", - "_readthedocs/html/", + "$READTHEDOCS_OUTPUT/html/", "docs/", escape_command=False, cwd=mock.ANY, @@ -1237,7 +1244,7 @@ def test_sphinx_fail_on_warning(self, load_yaml_config): "-D", "language=en", ".", - "../_readthedocs/html", + "$READTHEDOCS_OUTPUT/html", cwd=mock.ANY, bin_path=mock.ANY, ), @@ -1267,7 +1274,7 @@ def test_mkdocs_fail_on_warning(self, load_yaml_config): "build", "--clean", "--site-dir", - "../_readthedocs/html", + "$READTHEDOCS_OUTPUT/html", "--config-file", "docs/mkdocs.yaml", "--strict", # fail on warning flag @@ -1616,7 +1623,7 @@ def test_sphinx_builder(self, load_yaml_config, value, command): "-D", "language=en", ".", - "../_readthedocs/html", + "$READTHEDOCS_OUTPUT/html", cwd=mock.ANY, bin_path=mock.ANY, ), From 704436b486b430fe1dbeb560d33af0eb95a139ec Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 13 Feb 2023 17:04:25 +0100 Subject: [PATCH 09/10] Build: rename variables to make it more clear Use `absolute_container_output_dir` and `absolute_host_output_dir` to differentiate them in a clear way. --- readthedocs/doc_builder/backends/sphinx.py | 95 ++++++++++++++-------- 1 file changed, 59 insertions(+), 36 deletions(-) diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index 1d7e8a87b99..e8d1b4a51f0 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -49,9 +49,44 @@ class BaseSphinx(BaseBuilder): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.config_file = self.config.sphinx.configuration - self.absolute_output_dir = os.path.join( + + # We cannot use `$READTHEDOCS_OUTPUT` environment variable for + # `absolute_host_output_dir` because it's not defined in the host. So, + # we have to re-calculate its value. We will remove this limitation + # when we execute the whole building from inside the Docker container + # (instead behing a hybrid as it is now) + # + # We need to have two different paths that point to the exact same + # directory. How is that? The directory is mounted into a different + # location inside the container: + # + # 1. path in the host: /home/docs/checkouts/readthedocs.org/user_builds// + # 2. path in the container: /usr/src/app/checkouts/readthedocs.org/user_builds/b9cbc24c8841/test-builds/ + # + # Besides, the variable `$READTHEDOCS_OUTPUT` is not defined in the + # host, so we have to expand it using the full host's path. This + # variable cannot be used in cwd= due to a limitation of the Docker API + # (I guess) since I received an error when trying that. So, we have to + # fully expand it. + # + # That said, we need: + # + # * use the path in the host, for all the operations that are done via + # Python from the app (e.g. os.path.join, glob.glob, etc) + # + # * use the path in the container, for all the operations that are + # executed inside the container via Docker API using shell commands + self.absolute_host_output_dir = os.path.join( + os.path.join( + self.project.checkout_path(self.version.slug), + "_readthedocs/", + ), + self.relative_output_dir, + ) + self.absolute_container_output_dir = os.path.join( "$READTHEDOCS_OUTPUT", self.relative_output_dir ) + try: if not self.config_file: self.config_file = self.project.conf_file(self.version.slug) @@ -309,7 +344,7 @@ def build(self): # https://github.com/readthedocs/readthedocs.org/pull/9888#issuecomment-1384649346 ".", # Sphinx's output build directory (OUTPUTDIR) - self.absolute_output_dir, + self.absolute_container_output_dir, ] ) cmd_ret = self.run( @@ -334,19 +369,6 @@ def sphinx_parallel_arg(self): return ['-j', 'auto'] return [] - def _get_absolute_host_output_dir(self): - # NOTE: we cannot use `$READTHEDOCS_OUTPUT` environment variable here - # because it's not defined in the host. So, we have to re-calculate its - # value. We will remove this limitation when we execute the whole - # building from inside the Docker container (instead behing a hybrid as it is now) - readthedocs_output = os.path.join( - self.project.checkout_path(self.version.slug), "_readthedocs/" - ) - absolute_host_output_dir = os.path.join( - readthedocs_output, self.relative_output_dir - ) - return absolute_host_output_dir - class HtmlBuilder(BaseSphinx): relative_output_dir = "html" @@ -383,7 +405,7 @@ class LocalMediaBuilder(BaseSphinx): def _post_build(self): """Internal post build to create the ZIP file from the HTML output.""" target_file = os.path.join( - self.absolute_output_dir, + self.absolute_container_output_dir, # TODO: shouldn't this name include the name of the version as well? # It seems we were using the project's slug previously. # So, keeping it like that for now until we decide make that adjustment. @@ -399,7 +421,7 @@ def _post_build(self): dirname = f"{self.project.slug}-{self.version.slug}" self.run( "mv", - self.absolute_output_dir, + self.absolute_container_output_dir, str(tmp_dir / dirname), cwd=self.project_path, record=False, @@ -407,7 +429,7 @@ def _post_build(self): self.run( "mkdir", "--parents", - self.absolute_output_dir, + self.absolute_container_output_dir, cwd=self.project_path, record=False, ) @@ -431,12 +453,12 @@ def _post_build(self): """Internal post build to cleanup EPUB output directory and leave only one .epub file.""" temp_epub_file = f"/tmp/{self.project.slug}-{self.version.slug}.epub" target_file = os.path.join( - self.absolute_output_dir, + self.absolute_container_output_dir, f"{self.project.slug}.epub", ) epub_sphinx_filepaths = glob( - os.path.join(self._get_absolute_host_output_dir(), "*.epub") + os.path.join(self.absolute_host_output_dir, "*.epub") ) if epub_sphinx_filepaths: # NOTE: we currently support only one .epub per version @@ -448,14 +470,14 @@ def _post_build(self): self.run( "rm", "--recursive", - self.absolute_output_dir, + self.absolute_container_output_dir, cwd=self.project_path, record=False, ) self.run( "mkdir", "--parents", - self.absolute_output_dir, + self.absolute_container_output_dir, cwd=self.project_path, record=False, ) @@ -515,12 +537,12 @@ def build(self): # https://github.com/readthedocs/readthedocs.org/pull/9888#issuecomment-1384649346 ".", # Sphinx's output build directory (OUTPUTDIR) - self.absolute_output_dir, + self.absolute_container_output_dir, cwd=os.path.dirname(self.config_file), bin_path=self.python_env.venv_bin(), ) - tex_files = glob(os.path.join(self._get_absolute_host_output_dir(), "*.tex")) + tex_files = glob(os.path.join(self.absolute_host_output_dir, "*.tex")) if not tex_files: raise BuildUserError("No TeX files were found.") @@ -535,9 +557,7 @@ def _build_latexmk(self, cwd): # https://github.com/sphinx-doc/sphinx/blob/master/sphinx/texinputs/Makefile_t images = [] for extension in ("png", "gif", "jpg", "jpeg"): - images.extend( - Path(self._get_absolute_host_output_dir()).glob(f"*.{extension}") - ) + images.extend(Path(self.absolute_host_output_dir).glob(f"*.{extension}")) # FIXME: instead of checking by language here, what we want to check if # ``latex_engine`` is ``platex`` @@ -547,13 +567,13 @@ def _build_latexmk(self, cwd): # step. I don't know exactly why but most of the documentation that # I read differentiate this language from the others. I suppose # it's because it mix kanji (Chinese) with its own symbols. - pdfs = Path(self._get_absolute_host_output_dir()).glob("*.pdf") + pdfs = Path(self.absolute_host_output_dir).glob("*.pdf") for image in itertools.chain(images, pdfs): self.run( 'extractbb', image.name, - cwd=self._get_absolute_host_output_dir(), + cwd=self.absolute_host_output_dir, record=False, ) @@ -564,7 +584,7 @@ def _build_latexmk(self, cwd): self.run( 'cat', rcfile, - cwd=self._get_absolute_host_output_dir(), + cwd=self.absolute_host_output_dir, ) if self.build_env.command_class == DockerBuildCommand: @@ -592,7 +612,7 @@ def _build_latexmk(self, cwd): cls=latex_class, cmd=cmd, warn_only=True, - cwd=self._get_absolute_host_output_dir(), + cwd=self.absolute_host_output_dir, ) self.pdf_file_name = f'{self.project.slug}.pdf' @@ -608,14 +628,17 @@ def _post_build(self): # TODO: merge this with ePUB since it's pretty much the same temp_pdf_file = f"/tmp/{self.project.slug}-{self.version.slug}.pdf" target_file = os.path.join( - self.absolute_output_dir, + self.absolute_container_output_dir, self.pdf_file_name, ) # NOTE: we currently support only one .pdf per version - pdf_sphinx_filepath = os.path.join(self.absolute_output_dir, self.pdf_file_name) + pdf_sphinx_filepath = os.path.join( + self.absolute_container_output_dir, self.pdf_file_name + ) pdf_sphinx_filepath_host = os.path.join( - self._get_absolute_host_output_dir(), self.pdf_file_name + self.absolute_host_output_dir, + self.pdf_file_name, ) if os.path.exists(pdf_sphinx_filepath_host): self.run( @@ -628,14 +651,14 @@ def _post_build(self): self.run( "rm", "-r", - self.absolute_output_dir, + self.absolute_container_output_dir, cwd=self.project_path, record=False, ) self.run( "mkdir", "-p", - self.absolute_output_dir, + self.absolute_container_output_dir, cwd=self.project_path, record=False, ) From f805b2f810fdbd8cd1cbb116dc3b010735dc9b9a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 13 Feb 2023 18:21:38 +0100 Subject: [PATCH 10/10] Lint --- readthedocs/doc_builder/backends/sphinx.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index e8d1b4a51f0..da3be074982 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -60,8 +60,10 @@ def __init__(self, *args, **kwargs): # directory. How is that? The directory is mounted into a different # location inside the container: # - # 1. path in the host: /home/docs/checkouts/readthedocs.org/user_builds// - # 2. path in the container: /usr/src/app/checkouts/readthedocs.org/user_builds/b9cbc24c8841/test-builds/ + # 1. path in the host: + # /home/docs/checkouts/readthedocs.org/user_builds// + # 2. path in the container: + # /usr/src/app/checkouts/readthedocs.org/user_builds/b9cbc24c8841/test-builds/ # # Besides, the variable `$READTHEDOCS_OUTPUT` is not defined in the # host, so we have to expand it using the full host's path. This