Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_build_latex: move output to a separate output dir #11322

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Apr 12, 2023

The test fails with --random due to leftovers in app.outdir location.

Related: #11285

@AA-Turner

Copy link
Contributor

@jfbu jfbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I crucially rely on the output directory to remain there for my examination.

Please explain to me what --random is supposed to do so that I can see what is your exact problem. Mind that I currently only have access to a single core computer, in case this involves parallel builds.

The normal process constructs an output directory named latex. Inside this each of the 3 engines pdflatex lualatex and xelatex will have sub-directories named after them where one can find the auxiliary files of the (single) compilation pass. The latex directory iself holds all the style files from Sphinx and the Latexmk support files which are actually not used. There would be a problem if they were used after the fact because they do depend on the engine, so it is the last one i.e. 'xelatex' which leaves it imprint in latexmkrc. But the Latexmk from Makefile build is not used (cf compile_latex_document() in test_build_latex.py).

The only defect I see is that of the two docclasses 'howto' and 'manual' only the auxiliary files for engine run of the latter, i.e. 'manual', remain for examination.

I confirmed the output dir with their auxiliary files from first run remain in place after the 'howto' pass and hold for 'manual'. Is that your problem? Yes in theory I agree here that this is no good because changing document class in-between two runs can cause issues in theory.

edit indeed running 'manual' first and 'howto' next causes build failure, and this is surely what --random triggers.

With

tox -e py310 -- tests/test_build_latex.py::test_build_latex_doc

a new fresh test directory is created so there is no left-over effect from one such run to the next one.

I crucially rely on these output dir to remain in place for my everyday maintenance of Sphinx LaTeX. But while writing this I observed the above left-over effect from 'howto' to 'manual'. I tested this by adding

\AtEndDocument{\ifdefined\foo\error\fi\immediate\write\@mainaux{\gdef\string\foo{}}}

to the end of each of sphinx/texinputs/sphinx{manual,howto}.cls.

Ok, I propose this which fixes my problems but perhaps not yours ;-)

diff --git a/tests/test_build_latex.py b/tests/test_build_latex.py
index 394fa69fa..68ce274d4 100644
--- a/tests/test_build_latex.py
+++ b/tests/test_build_latex.py
@@ -5,7 +5,7 @@ import re
 import subprocess
 from itertools import product
 from pathlib import Path
-from shutil import copyfile, rmtree
+from shutil import copyfile
 from subprocess import CalledProcessError
 
 import pytest
@@ -51,17 +51,20 @@ def kpsetest(*filenames):
 
 
 # compile latex document with app.config.latex_engine
-def compile_latex_document(app, filename='python.tex'):
+def compile_latex_document(app, filename='python.tex', docclass='manual'):
     # now, try to run latex over it
     try:
         with chdir(app.outdir):
-            ensuredir(app.config.latex_engine)
+            # name latex output-directory according to both engine and docclass
+            # to avoid reuse of auxiliary files by one docclass from another
+            latex_outputdir = app.config.latex_engine + docclass
+            ensuredir(latex_outputdir)
             # keep a copy of latex file for this engine in case test fails
-            copyfile(filename, app.config.latex_engine + '/' + filename)
+            copyfile(filename, latex_outputdir + '/' + filename)
             args = [app.config.latex_engine,
                     '--halt-on-error',
                     '--interaction=nonstopmode',
-                    '-output-directory=%s' % app.config.latex_engine,
+                    '-output-directory=%s' % latex_outputdir,
                     filename]
             subprocess.run(args, capture_output=True, check=True)
     except OSError as exc:  # most likely the latex executable was not found
@@ -117,8 +120,7 @@ def test_build_latex_doc(app, status, warning, engine, docclass):
     # file from latex_additional_files
     assert (app.outdir / 'svgimg.svg').isfile()
 
-    compile_latex_document(app, 'sphinxtests.tex')
-    rmtree(app.outdir)
+    compile_latex_document(app, 'sphinxtests.tex', docclass)
 
 
 @pytest.mark.sphinx('latex')

Does it by any luck address your issue? (edit yes it should! let's be brash)

@jfbu
Copy link
Contributor

jfbu commented Apr 12, 2023

AH OK I do understand now that your problem with --random is exactly my problem as well. And it is fixed by my proposed patch! Can you update your branch and I will merge, thanks. Indeed, a 'howto' docclass run auxiliary files break the 'manual' docclass ones.

edit: well sorry it is actually the opposite. It fails when 'manual' is executed first, and the 'howto'. Strangely I was under silly impression 'manual' was run first due to not looking carefully enought at DOCCLASSES in test_build_latex.py and from the fact that pytest+tox reports the slowest duration and I hence always saw 'manual' first then 'howto'. Sorry. I will edit my earlier comment to clean up.

Maybe we can also add this extra diff

diff --git a/tests/test_build_latex.py b/tests/test_build_latex.py
index 394fa69fa..c1db398bb 100644
--- a/tests/test_build_latex.py
+++ b/tests/test_build_latex.py
@@ -27,7 +27,7 @@ except ImportError:
     from sphinx.util.osutil import _chdir as chdir
 
 LATEX_ENGINES = ['pdflatex', 'lualatex', 'xelatex']
-DOCCLASSES = ['howto', 'manual']
+DOCCLASSES = ['manual', 'howto']
 STYLEFILES = ['article.cls', 'fancyhdr.sty', 'titlesec.sty', 'amsmath.sty',
               'framed.sty', 'color.sty', 'fancyvrb.sty',
               'fncychap.sty', 'geometry.sty', 'kvoptions.sty', 'hyperref.sty',

as a regression test as without the other changes, it causes test failures. No need to worry about order of engines as I explained the left-overs in the latexmkrc are of no consequence because the test only does one (pdf,lua,xe)latex run and no "makeindex" or other things which could be broken by specific tailored helper files in this directory.

Related: sphinx-doc#11285
Co-authored-by: Jean-François B <2589111+jfbu@users.noreply.github.com>
@marxin marxin changed the title test_build_latex: clean-up Latex files test_build_latex: move output to a separate output dir Apr 13, 2023
@marxin marxin requested a review from jfbu April 13, 2023 04:20
@marxin
Copy link
Contributor Author

marxin commented Apr 13, 2023

Thanks @jfbu for the suggestion, it works fine!

Copy link
Contributor

@jfbu jfbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @marxin and glad my lengthy discourse can be summarized in two words "it works". I will merge soon.

@jfbu jfbu merged commit b6e6805 into sphinx-doc:master Apr 13, 2023
@marxin marxin deleted the fix-test_build_latex branch April 13, 2023 07:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants