From a2218b373ff7e6982281ead2d4630f7572b81bcd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Mar 2023 20:17:27 +0100 Subject: [PATCH 1/5] Build: fail PDF command (`latexmk`) if exit code != 0 We were forcing `exit_code=0` if we detected that `Output written on (.*?)` was found in the standard output of the `latexmk` command. This was producing confusions to our users because the PDF generation failed but we were considering it successful. Now, we are relying on the exit code itself as we do for all the other commands making sure the command succeeded before proceding with the build. Now, if the PDF command fails, we fail the build completely. --- readthedocs/doc_builder/backends/sphinx.py | 37 ++-------------------- readthedocs/doc_builder/constants.py | 3 -- readthedocs/projects/tests/mockers.py | 7 ---- 3 files changed, 2 insertions(+), 45 deletions(-) diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index 221d6952f31..f743a13fb22 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -26,8 +26,6 @@ from readthedocs.projects.utils import safe_write from ..base import BaseBuilder -from ..constants import PDF_RE -from ..environments import BuildCommand, DockerBuildCommand from ..exceptions import BuildUserError from ..signals import finalize_sphinx_context_data @@ -488,30 +486,6 @@ def _post_build(self): ) -class LatexBuildCommand(BuildCommand): - - """Ignore LaTeX exit code if there was file output.""" - - def run(self): - super().run() - # Force LaTeX exit code to be a little more optimistic. If LaTeX - # reports an output file, let's just assume we're fine. - if PDF_RE.search(self.output): - self.exit_code = 0 - - -class DockerLatexBuildCommand(DockerBuildCommand): - - """Ignore LaTeX exit code if there was file output.""" - - def run(self): - super().run() - # Force LaTeX exit code to be a little more optimistic. If LaTeX - # reports an output file, let's just assume we're fine. - if PDF_RE.search(self.output): - self.exit_code = 0 - - class PdfBuilder(BaseSphinx): """Builder to generate PDF documentation.""" @@ -589,11 +563,6 @@ def _build_latexmk(self, cwd): cwd=self.absolute_host_output_dir, ) - if self.build_env.command_class == DockerBuildCommand: - latex_class = DockerLatexBuildCommand - else: - latex_class = LatexBuildCommand - cmd = [ 'latexmk', '-r', @@ -610,10 +579,8 @@ def _build_latexmk(self, cwd): '-interaction=nonstopmode', ] - cmd_ret = self.build_env.run_command_class( - cls=latex_class, - cmd=cmd, - warn_only=True, + cmd_ret = self.run( + *cmd, cwd=self.absolute_host_output_dir, ) diff --git a/readthedocs/doc_builder/constants.py b/readthedocs/doc_builder/constants.py index b034479b480..d1400615bc8 100644 --- a/readthedocs/doc_builder/constants.py +++ b/readthedocs/doc_builder/constants.py @@ -1,15 +1,12 @@ """Doc build constants.""" -import re import structlog from django.conf import settings log = structlog.get_logger(__name__) -PDF_RE = re.compile('Output written on (.*?)') - # Docker DOCKER_SOCKET = settings.DOCKER_SOCKET DOCKER_VERSION = settings.DOCKER_VERSION diff --git a/readthedocs/projects/tests/mockers.py b/readthedocs/projects/tests/mockers.py index e700bd66663..8371f1b4143 100644 --- a/readthedocs/projects/tests/mockers.py +++ b/readthedocs/projects/tests/mockers.py @@ -58,13 +58,6 @@ def _mock_artifact_builders(self): "project-slug.pdf", ) - self.patches['builder.pdf.LatexBuildCommand.run'] = mock.patch( - 'readthedocs.doc_builder.backends.sphinx.LatexBuildCommand.run', - return_value=mock.MagicMock(output='stdout', successful=True) - ) - # self.patches['builder.pdf.LatexBuildCommand.output'] = mock.patch( - # 'readthedocs.doc_builder.backends.sphinx.LatexBuildCommand.output', - # ) self.patches['builder.pdf.glob'] = mock.patch( 'readthedocs.doc_builder.backends.sphinx.glob', return_value=['output.file'], From 994430feb0290516aee88b5b4d6bc89dbb1e4f0f Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Mar 2023 20:42:31 +0100 Subject: [PATCH 2/5] Test: adapt tests to the changes on PDF building --- .../projects/tests/test_build_tasks.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 19b1f5c5803..390f85c0141 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -723,6 +723,18 @@ def test_build_commands_executed( bin_path=mock.ANY, ), mock.call("cat", "latexmkrc", cwd=mock.ANY), + mock.call( + "latexmk", + "-r", + "latexmkrc", + "-pdf", + "-f", + "-dvi-", + "-ps-", + "-jobname=project", + "-interaction=nonstopmode", + cwd=mock.ANY, + ), # NOTE: pdf `mv` commands and others are not here because the # PDF resulting file is not found in the process (`_post_build`) mock.call( @@ -794,6 +806,14 @@ def test_build_commands_executed( record=False, demux=True, ), + mock.call( + "test", + "-x", + "_build/html", + record=False, + demux=True, + cwd=mock.ANY, + ), ] ) From 2a2870b08b745aa172ba6889412e4c7974e62261 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 7 Mar 2023 09:51:14 +0100 Subject: [PATCH 3/5] Build: specific message for PDF generation failure --- readthedocs/doc_builder/backends/sphinx.py | 18 +++++++++++------- readthedocs/doc_builder/exceptions.py | 7 +++++++ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index f743a13fb22..0c1a409c520 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -579,14 +579,18 @@ def _build_latexmk(self, cwd): '-interaction=nonstopmode', ] - cmd_ret = self.run( - *cmd, - cwd=self.absolute_host_output_dir, - ) - - self.pdf_file_name = f'{self.project.slug}.pdf' + try: + cmd_ret = self.run( + *cmd, + cwd=self.absolute_host_output_dir, + ) + self.pdf_file_name = f"{self.project.slug}.pdf" + return cmd_ret.successful - return cmd_ret.successful + # Catch the exception and re-raise it with a specific message + except BuildUserError: + raise BuildUserError(BuildUserError.PDF_COMMAND_FAILED) + return False def _post_build(self): """Internal post build to cleanup PDF output directory and leave only one .pdf file.""" diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index b48df350f97..6c645f0f341 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -52,6 +52,13 @@ class BuildUserError(BuildBaseException): "and it is not currently supported. " 'Please, remove all the files but the "{artifact_type}" your want to upload.' ) + PDF_COMMAND_FAILED = gettext_noop( + "PDF generation failed. " + "Please double check the command's output looking for errors and fix them. " + "Note it's possible the PDF has been always failing silently in your project, " + "but due to latest changes, " + "Read the Docs now makes it explicit and fail the whole build." + ) class BuildUserSkip(BuildUserError): From 492c4674bd3752f15ccd5bfaf87ba96ea006dfdd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 7 Mar 2023 15:33:14 +0100 Subject: [PATCH 4/5] Better user message Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com> --- readthedocs/doc_builder/exceptions.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index 6c645f0f341..5fc4446a3fa 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -54,10 +54,10 @@ class BuildUserError(BuildBaseException): ) PDF_COMMAND_FAILED = gettext_noop( "PDF generation failed. " - "Please double check the command's output looking for errors and fix them. " - "Note it's possible the PDF has been always failing silently in your project, " - "but due to latest changes, " - "Read the Docs now makes it explicit and fail the whole build." + "The build log below contains information on what errors caused the failure." + "Our code has recently changed to fail the entire build on PDF errors, " + "where we used to pass the build when a PDF was created." + "Please contact us if you need help understanding this error." ) From 9027fd070711987b4e4a59ab5ff38ff66af6d9f5 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 7 Mar 2023 19:11:27 +0100 Subject: [PATCH 5/5] Update readthedocs/doc_builder/exceptions.py --- readthedocs/doc_builder/exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index 5fc4446a3fa..b9158acff43 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -54,7 +54,7 @@ class BuildUserError(BuildBaseException): ) PDF_COMMAND_FAILED = gettext_noop( "PDF generation failed. " - "The build log below contains information on what errors caused the failure." + "The build log below contains information on what errors caused the failure." "Our code has recently changed to fail the entire build on PDF errors, " "where we used to pass the build when a PDF was created." "Please contact us if you need help understanding this error."