Skip to content

Use latexmk if Sphinx > 1.6 #5656

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
May 16, 2019
Merged

Use latexmk if Sphinx > 1.6 #5656

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ec7050b
Use Sphinx's version from user's venv to decide usage of latexmk
humitos May 2, 2019
699cba9
Remove USE_PDF_LATEXMK feature flag
humitos May 2, 2019
dc843ce
Allow to not escape the command on `get_wrapped_command`
humitos May 2, 2019
fd13ee5
Do not record the command that checks for Sphinx version
humitos May 2, 2019
0f5080e
Allow to extend BuildCommand class with **kwargs
humitos May 6, 2019
3de6972
Update tests to add the new call into the mocks
humitos May 6, 2019
ebb1325
Make `venv_sphinx_version` compatible with local and docker builder
humitos May 8, 2019
9d3a71a
Small comments to make it clear when debugging
humitos May 8, 2019
f9e24c6
Use exit code to make the decision instead of parsing command output
humitos May 15, 2019
3272a0c
Return proper output/exit_code in mocks
humitos May 15, 2019
65b44e0
Update readthedocs/doc_builder/backends/sphinx.py
humitos May 16, 2019
204c020
Remove unused import
humitos May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions docs/guides/feature-flags.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ or disable one or more of these featured flags for a particular project.
Available Flags
---------------

``USE_PDF_LATEXMK``: :featureflags:`USE_PDF_LATEXMK`

``USE_SPHINX_LATEST``: :featureflags:`USE_SPHINX_LATEST`

``ALLOW_DEPRECATED_WEBHOOKS``: :featureflags:`ALLOW_DEPRECATED_WEBHOOKS`
Expand Down
7 changes: 0 additions & 7 deletions docs/guides/pdf-non-ascii-languages.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,6 @@
Build PDF format for non-ASCII languages
========================================


.. warning::

To be able to follow this guide and build PDF with this method,
you need to ask the Read the Docs core team to enable ``USE_PDF_LATEXMK`` :doc:`feature flag </guides/feature-flags>` in your project.
Please, `open an issue`_ in our repository asking for this, and wait for one of the core team to enable it.

.. _open an issue: https://github.com/rtfd/readthedocs.org/issues/new

Sphinx offers different `LaTeX engines`_ that support Unicode characters and non-ASCII languages,
Expand Down
38 changes: 33 additions & 5 deletions readthedocs/doc_builder/backends/sphinx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@
import logging
import os
import shutil
import sys
import zipfile
from glob import glob
from pathlib import Path

from django.conf import settings
from django.template import loader as template_loader
from django.template.loader import render_to_string
from packaging.version import Version
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks unused now.


from readthedocs.builds import utils as version_utils
from readthedocs.projects.exceptions import ProjectConfigurationError
Expand Down Expand Up @@ -149,9 +149,6 @@ def get_config_params(self):
'dont_overwrite_sphinx_context': self.project.has_feature(
Feature.DONT_OVERWRITE_SPHINX_CONTEXT,
),
'use_pdf_latexmk': self.project.has_feature(
Feature.USE_PDF_LATEXMK,
),
}

finalize_sphinx_context_data.send(
Expand Down Expand Up @@ -232,6 +229,34 @@ def build(self):
)
return cmd_ret.successful

def venv_sphinx_version(self):
"""
Execute Python from the venv to query the ``sphinx`` version.

Example,

>>> import sphinx
>>> print(sphinx.__version__)
'2.0.0'

In this case, the output will be ``2.0.0``.
"""

command = [
self.python_env.venv_bin(filename='python'),
'-c'
Copy link
Member

Choose a reason for hiding this comment

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

You are missing a comma here

'"import sphinx; print(sphinx.__version__)"',
Copy link
Member

Choose a reason for hiding this comment

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

when DOCKER_ENABLED is false, this will break. Because of the double quotes

>>> proc = subprocess.Popen(['python3', '-c', 'import sphinx; print(sphinx.__version__)'], stdou
t=subprocess.PIPE)
>>> proc.communicate()
(b'1.8.5\n', None)
>>> proc = subprocess.Popen(['python3', '-c', '"import sphinx; print(sphinx.__version__)"'], std
out=subprocess.PIPE)
>>> proc.communicate()
(b'', None)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for raising this.

I made it work by using shell=True and flattening the command when shell=True using the helper that we had there.

I like how the ended now. On BuildCommand it uses the shell=True attribute and flatten the command to be compatible. On DockerBuildCommand it uses escape_command=False to not escape the command.

It works in both builder backends.

]

cmd_ret = self.run(
*command,
bin_path=self.python_env.venv_bin(),
cwd=self.project.checkout_path(self.version.slug),
escape_command=False,
record=False,
)
return cmd_ret.output


class HtmlBuilder(BaseSphinx):
type = 'sphinx'
Expand Down Expand Up @@ -395,7 +420,10 @@ def build(self):
raise BuildEnvironmentError('No TeX files were found')

# Run LaTeX -> PDF conversions
if self.project.has_feature(Feature.USE_PDF_LATEXMK):
# Build PDF with ``latexmk`` if Sphinx > 1.6, otherwise fallback to
# ``pdflatex`` to support old versions
sphinx_version = Version(self.venv_sphinx_version())
if sphinx_version > Version('1.6'):
return self._build_latexmk(cwd, latex_cwd)

return self._build_pdflatex(tex_files, latex_cwd)
Expand Down
44 changes: 28 additions & 16 deletions readthedocs/doc_builder/environments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class BuildCommand(BuildCommandResultMixin):
:param build_env: build environment to use to execute commands
:param bin_path: binary path to add to PATH resolution
:param description: a more grokable description of the command being run
:param kwargs: allow to subclass this class and extend it
"""

def __init__(
Expand All @@ -97,6 +98,7 @@ def __init__(
bin_path=None,
description=None,
record_as_success=False,
**kwargs,
):
self.command = command
self.shell = shell
Expand Down Expand Up @@ -296,14 +298,20 @@ class DockerBuildCommand(BuildCommand):
Build command to execute in docker container
"""

def run(self):
def __init__(self, *args, escape_command=True, **kwargs):
"""
Execute command in existing Docker container.
Override default to extend behavior.

:param cmd_input: input to pass to command in STDIN
:type cmd_input: str
:param combine_output: combine STDERR into STDOUT
:param escape_command: whether escape special chars the command before
executing it in the container. This should only be disabled on
trusted or internal commands.
:type escape_command: bool
"""
self.escape_command = escape_command
Copy link
Member

Choose a reason for hiding this comment

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

Feels like something that can be moved to the parent class. I think (not sure) that the same behavior for this in local command is shell=True

https://docs.python.org/3.6/library/subprocess.html

On POSIX with shell=True, the shell defaults to /bin/sh. If args is a string, the string specifies the command to execute through the shell. This means that the string must be formatted exactly as it would be when typed at the shell prompt. This includes, for example, quoting or backslash escaping filenames with spaces in them. If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself...

super(DockerBuildCommand, self).__init__(*args, **kwargs)

def run(self):
"""Execute command in existing Docker container."""
log.info(
"Running in container %s: '%s' [%s]",
self.build_env.container_id,
Expand Down Expand Up @@ -352,13 +360,15 @@ def run(self):

def get_wrapped_command(self):
"""
Escape special bash characters in command to wrap in shell.
Wrap command in a shell and optionally escape special bash characters.

In order to set the current working path inside a docker container, we
need to wrap the command in a shell call manually. Some characters will
be interpreted as shell characters without escaping, such as: ``pip
install requests<0.8``. This escapes a good majority of those
characters.
need to wrap the command in a shell call manually.

Some characters will be interpreted as shell characters without
escaping, such as: ``pip install requests<0.8``. When passing
``escape_command=True`` in the init method this escapes a good majority
of those characters.
"""
bash_escape_re = re.compile(
r"([\t\ \!\"\#\$\&\'\(\)\*\:\;\<\>\?\@"
Expand All @@ -367,16 +377,18 @@ def get_wrapped_command(self):
prefix = ''
if self.bin_path:
prefix += 'PATH={}:$PATH '.format(self.bin_path)

command = (
' '.join([
bash_escape_re.sub(r'\\\1', part) if self.escape_command else part
for part in self.command
])
)
return (
"/bin/sh -c 'cd {cwd} && {prefix}{cmd}'".format(
cwd=self.cwd,
prefix=prefix,
cmd=(
' '.join([
bash_escape_re.sub(r'\\\1', part)
for part in self.command
])
),
cmd=command,
)
)

Expand Down
2 changes: 0 additions & 2 deletions readthedocs/doc_builder/templates/doc_builder/conf.py.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ if 'extensions' in globals():
else:
extensions = ["readthedocs_ext.readthedocs"]

{% if use_pdf_latexmk %}
project_language = '{{ project.language }}'

# User's Sphinx configurations
Expand Down Expand Up @@ -173,4 +172,3 @@ if chinese:
latex_elements = latex_elements_user or latex_elements_rtd
elif japanese:
latex_engine = latex_engine_user or 'platex'
{% endif %}
2 changes: 0 additions & 2 deletions readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1337,12 +1337,10 @@ def add_features(sender, **kwargs):
DONT_SHALLOW_CLONE = 'dont_shallow_clone'
USE_TESTING_BUILD_IMAGE = 'use_testing_build_image'
SHARE_SPHINX_DOCTREE = 'share_sphinx_doctree'
USE_PDF_LATEXMK = 'use_pdf_latexmk'
DEFAULT_TO_MKDOCS_0_17_3 = 'default_to_mkdocs_0_17_3'

FEATURES = (
(USE_SPHINX_LATEST, _('Use latest version of Sphinx')),
(USE_PDF_LATEXMK, _('Use latexmk to build the PDF')),
(ALLOW_DEPRECATED_WEBHOOKS, _('Allow deprecated webhook views')),
(PIP_ALWAYS_UPGRADE, _('Always run pip install --upgrade')),
(SKIP_SUBMODULES, _('Skip git submodule checkout')),
Expand Down
6 changes: 4 additions & 2 deletions readthedocs/rtd_tests/tests/test_builds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ def test_build_pdf_latex_failures(self, load_config):
returns = [
((b'', b''), 0), # sphinx-build html
((b'', b''), 0), # sphinx-build pdf
((b'1.5', b''), 0), # sphinx version check
((b'', b''), 1), # latex
((b'', b''), 0), # makeindex
((b'', b''), 0), # latex
Expand All @@ -236,7 +237,7 @@ def test_build_pdf_latex_failures(self, load_config):

with build_env:
task.build_docs()
self.assertEqual(self.mocks.popen.call_count, 7)
self.assertEqual(self.mocks.popen.call_count, 8)
self.assertTrue(build_env.failed)

@mock.patch('readthedocs.doc_builder.config.load_config')
Expand Down Expand Up @@ -271,6 +272,7 @@ def test_build_pdf_latex_not_failure(self, load_config):
returns = [
((b'', b''), 0), # sphinx-build html
((b'', b''), 0), # sphinx-build pdf
((b'1.6', b''), 0), # sphinx version check
((b'Output written on foo.pdf', b''), 1), # latex
((b'', b''), 0), # makeindex
((b'', b''), 0), # latex
Expand All @@ -287,7 +289,7 @@ def test_build_pdf_latex_not_failure(self, load_config):

with build_env:
task.build_docs()
self.assertEqual(self.mocks.popen.call_count, 7)
self.assertEqual(self.mocks.popen.call_count, 8)
self.assertTrue(build_env.successful)

@mock.patch('readthedocs.projects.tasks.api_v2')
Expand Down