Build: use environment variable $READTHEDOCS_OUTPUT to define outpu…

…t directory (#9913)

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

* Log environment during build commands

* Love the environment in the container build as well

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

* Build: use `$READTHEDOCS_OUTPUT` variable for MkDocs builder

* Log: do not log the environment

It could contain some private data.

* 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).

* Test: update tests to use the new variable

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

* Lint

---------

Co-authored-by: Eric Holscher <[email protected]>

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),
         ]

readthedocs/doc_builder/backends/sphinx.py

@@ -42,16 +42,53 @@ 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/<format>")
+    # 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)
+
+        # 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/<project>/
+        #  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,9 +346,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)
-                ),
+                self.absolute_container_output_dir,
             ]
         )
         cmd_ret = self.run(
@@ -338,7 +373,7 @@ def sphinx_parallel_arg(self):
 
 
 class HtmlBuilder(BaseSphinx):
-    relative_output_dir = "_readthedocs/html"
+    relative_output_dir = "html"
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -367,18 +402,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_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.
+            f"{self.project.slug}.zip",
         )
 
         # **SECURITY CRITICAL: Advisory GHSA-hqwg-gjqw-h5wg**
@@ -390,15 +423,15 @@ def _post_build(self):
         dirname = f"{self.project.slug}-{self.version.slug}"
         self.run(
             "mv",
-            self.relative_output_dir,
+            self.absolute_container_output_dir,
             str(tmp_dir / dirname),
             cwd=self.project_path,
             record=False,
         )
         self.run(
             "mkdir",
             "--parents",
-            self.relative_output_dir,
+            self.absolute_container_output_dir,
             cwd=self.project_path,
             record=False,
         )
@@ -416,17 +449,19 @@ 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."""
         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.absolute_output_dir, "*.epub"))
+        epub_sphinx_filepaths = glob(
+            os.path.join(self.absolute_host_output_dir, "*.epub")
+        )
         if epub_sphinx_filepaths:
             # NOTE: we currently support only one .epub per version
             epub_filepath = epub_sphinx_filepaths[0]
@@ -437,14 +472,14 @@ def _post_build(self):
             self.run(
                 "rm",
                 "--recursive",
-                self.relative_output_dir,
+                self.absolute_container_output_dir,
                 cwd=self.project_path,
                 record=False,
             )
             self.run(
                 "mkdir",
                 "--parents",
-                self.relative_output_dir,
+                self.absolute_container_output_dir,
                 cwd=self.project_path,
                 record=False,
             )
@@ -481,7 +516,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
 
@@ -504,14 +539,12 @@ 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)
-            ),
+            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.absolute_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.")
 
@@ -526,7 +559,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.absolute_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``
@@ -536,13 +569,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.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.absolute_host_output_dir,
                 record=False,
             )
 
@@ -553,7 +586,7 @@ def _build_latexmk(self, cwd):
         self.run(
             'cat',
             rcfile,
-            cwd=self.absolute_output_dir,
+            cwd=self.absolute_host_output_dir,
         )
 
         if self.build_env.command_class == DockerBuildCommand:
@@ -581,7 +614,7 @@ def _build_latexmk(self, cwd):
             cls=latex_class,
             cmd=cmd,
             warn_only=True,
-            cwd=self.absolute_output_dir,
+            cwd=self.absolute_host_output_dir,
         )
 
         self.pdf_file_name = f'{self.project.slug}.pdf'
@@ -597,13 +630,19 @@ 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)
-        if os.path.exists(pdf_sphinx_filepath):
+        pdf_sphinx_filepath = os.path.join(
+            self.absolute_container_output_dir, self.pdf_file_name
+        )
+        pdf_sphinx_filepath_host = os.path.join(
+            self.absolute_host_output_dir,
+            self.pdf_file_name,
+        )
+        if os.path.exists(pdf_sphinx_filepath_host):
             self.run(
                 "mv",
                 pdf_sphinx_filepath,
@@ -614,14 +653,14 @@ def _post_build(self):
             self.run(
                 "rm",
                 "-r",
-                self.relative_output_dir,
+                self.absolute_container_output_dir,
                 cwd=self.project_path,
                 record=False,
             )
             self.run(
                 "mkdir",
                 "-p",
-                self.relative_output_dir,
+                self.absolute_container_output_dir,
                 cwd=self.project_path,
                 record=False,
             )

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
 

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