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

Implement sphinx key from v2 config #4482

Merged
merged 21 commits into from
Sep 7, 2018
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
13 changes: 11 additions & 2 deletions readthedocs/config/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,14 @@ def build(self):
def doctype(self):
return self.defaults['doctype']

@property
def sphinx(self):
return Sphinx(
builder=self.doctype,
configuration=self.defaults['sphinx_configuration'],
fail_on_warning=False,
)


class BuildConfigV2(BuildConfigBase):

Expand Down Expand Up @@ -959,7 +967,8 @@ def get_configuration_class(version):
version = int(version)
return configurations_class[version]
except (KeyError, ValueError):
raise ConfigError(
'Invalid version of the configuration file',
raise InvalidConfig(
'version',
code=VERSION_INVALID,
error_message='Invalid version of the configuration file',
)
9 changes: 7 additions & 2 deletions readthedocs/doc_builder/backends/sphinx.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ def build(self):
]
if self._force:
build_command.append('-E')
if self.config.sphinx.fail_on_warning:
build_command.append('-W')
build_command.extend([
'-b',
self.sphinx_builder,
Expand All @@ -209,9 +211,12 @@ def build(self):
'.',
self.sphinx_build_dir,
])
config_file = self.config.sphinx.configuration
cmd_ret = self.run(
*build_command, cwd=project.conf_dir(self.version.slug),
bin_path=self.python_env.venv_bin())
*build_command,
cwd=os.path.dirname(config_file),
bin_path=self.python_env.venv_bin()
)
return cmd_ret.successful


Expand Down
1 change: 1 addition & 0 deletions readthedocs/doc_builder/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def __init__(self, build_env, python_env, force=False):
self.python_env = python_env
self.version = build_env.version
self.project = build_env.project
self.config = python_env.config
self._force = force
self.target = self.project.artifact_path(
version=self.version.slug, type_=self.type)
Expand Down
5 changes: 4 additions & 1 deletion readthedocs/doc_builder/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ def load_yaml_config(version):
python_version = 3 if project.python_interpreter == 'python3' else 2
allow_v2 = project.has_feature(Feature.ALLOW_V2_CONFIG_FILE)
try:
sphinx_configuration = version.get_conf_py_path()
sphinx_configuration = path.join(
version.get_conf_py_path(),
'conf.py'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why this change is needed. Shouldn't the full path have already been returned by version.get_conf_py_path()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was bitten by the name here too, this only returns the directory name. There is a test unskipped that covers this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

huh. is there a reason why this was working then? It seems like it shouldn't have worked at all if it was just the directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Also currently we are reading this from the database, that's why the builds don't fail

except ProjectConfigurationError:
sphinx_configuration = None

Expand Down
2 changes: 1 addition & 1 deletion readthedocs/projects/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ def build_docs(self):

def build_docs_html(self):
"""Build HTML docs."""
html_builder = get_builder_class(self.project.documentation_type)(
html_builder = get_builder_class(self.config.doctype)(
build_env=self.build_env,
python_env=self.python_env,
)
Expand Down
54 changes: 46 additions & 8 deletions readthedocs/rtd_tests/tests/test_config_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ def test_requirements_file(self, load_config):
self.assertEqual(config.python.requirements, '__init__.py')


@pytest.mark.skip
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests are un-skipped here

@pytest.mark.django_db
@mock.patch('readthedocs.projects.models.Project.checkout_path')
class TestLoadConfigV2(object):
Expand Down Expand Up @@ -290,12 +289,10 @@ def test_report_using_invalid_version(self, checkout_path, tmpdir):

@pytest.mark.parametrize('config', [{}, {'formats': []}])
@patch('readthedocs.projects.models.Project.repo_nonblockinglock', new=MagicMock())
@patch('readthedocs.doc_builder.backends.sphinx.SearchBuilder.build')
@patch('readthedocs.doc_builder.backends.sphinx.HtmlBuilder.build')
@patch('readthedocs.doc_builder.backends.sphinx.HtmlBuilder.append_conf')
def test_build_formats_default_empty(
self, append_conf, html_build, search_build,
checkout_path, config, tmpdir):
self, append_conf, html_build, checkout_path, config, tmpdir):
"""
The default value for formats is [], which means no extra
formats are build.
Expand All @@ -304,22 +301,26 @@ def test_build_formats_default_empty(
self.create_config_file(tmpdir, config)

update_docs = self.get_update_docs_task()
python_env = Virtualenv(
version=self.version,
build_env=update_docs.build_env,
config=config
)
update_docs.python_env = python_env
outcomes = update_docs.build_docs()

# No extra formats were triggered
assert outcomes['html']
assert outcomes['search']
assert not outcomes['localmedia']
assert not outcomes['pdf']
assert not outcomes['epub']

@patch('readthedocs.projects.models.Project.repo_nonblockinglock', new=MagicMock())
@patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs_class')
@patch('readthedocs.doc_builder.backends.sphinx.SearchBuilder.build')
@patch('readthedocs.doc_builder.backends.sphinx.HtmlBuilder.build')
@patch('readthedocs.doc_builder.backends.sphinx.HtmlBuilder.append_conf')
def test_build_formats_only_pdf(
self, append_conf, html_build, search_build, build_docs_class,
self, append_conf, html_build, build_docs_class,
checkout_path, tmpdir):
"""
Only the pdf format is build.
Expand All @@ -328,11 +329,17 @@ def test_build_formats_only_pdf(
self.create_config_file(tmpdir, {'formats': ['pdf']})

update_docs = self.get_update_docs_task()
python_env = Virtualenv(
version=self.version,
build_env=update_docs.build_env,
config=update_docs.config
)
update_docs.python_env = python_env

outcomes = update_docs.build_docs()

# Only pdf extra format was triggered
assert outcomes['html']
assert outcomes['search']
build_docs_class.assert_called_with('sphinx_pdf')
assert outcomes['pdf']
assert not outcomes['localmedia']
Expand Down Expand Up @@ -642,6 +649,35 @@ def test_sphinx_configuration_default(
append_conf.assert_called_once()
move.assert_called_once()

@patch('readthedocs.doc_builder.backends.sphinx.BaseSphinx.move')
@patch('readthedocs.doc_builder.backends.sphinx.BaseSphinx.append_conf')
@patch('readthedocs.doc_builder.backends.sphinx.BaseSphinx.run')
def test_sphinx_configuration_default(
self, run, append_conf, move, checkout_path, tmpdir):
"""Should be default to find a conf.py file."""
checkout_path.return_value = str(tmpdir)

apply_fs(tmpdir, {'conf.py': ''})
self.create_config_file(tmpdir, {})
self.project.conf_py_file = ''
self.project.save()

update_docs = self.get_update_docs_task()
config = update_docs.config
python_env = Virtualenv(
version=self.version,
build_env=update_docs.build_env,
config=config
)
update_docs.python_env = python_env

update_docs.build_docs_html()

args, kwargs = run.call_args
assert kwargs['cwd'] == str(tmpdir)
append_conf.assert_called_once()
move.assert_called_once()

@patch('readthedocs.doc_builder.backends.sphinx.BaseSphinx.move')
@patch('readthedocs.doc_builder.backends.sphinx.BaseSphinx.append_conf')
@patch('readthedocs.doc_builder.backends.sphinx.BaseSphinx.run')
Expand Down Expand Up @@ -716,6 +752,7 @@ def test_sphinx_fail_on_warning(
append_conf.assert_called_once()
move.assert_called_once()

@pytest.mark.skip
@patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.move')
@patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.append_conf')
@patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.run')
Expand Down Expand Up @@ -754,6 +791,7 @@ def test_mkdocs_configuration(
append_conf.assert_called_once()
move.assert_called_once()

@pytest.mark.skip
@patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.move')
@patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.append_conf')
@patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.run')
Expand Down
69 changes: 55 additions & 14 deletions readthedocs/rtd_tests/tests/test_doc_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
import tempfile
from collections import namedtuple

import mock
import pytest
import yaml
import mock
from django.test import TestCase
from django.test.utils import override_settings
from django_dynamic_fixture import get
Expand All @@ -17,6 +17,7 @@
from readthedocs.builds.models import Version
from readthedocs.doc_builder.backends.mkdocs import BaseMkdocs, MkdocsHTML
from readthedocs.doc_builder.backends.sphinx import BaseSphinx
from readthedocs.doc_builder.python_environments import Virtualenv
from readthedocs.projects.exceptions import ProjectConfigurationError
from readthedocs.projects.models import Project

Expand All @@ -29,13 +30,12 @@ def setUp(self):
self.project = Project.objects.get(slug='pip')
self.version = self.project.versions.first()

build_env = namedtuple('project', 'version')
build_env.project = self.project
build_env.version = self.version
self.build_env = namedtuple('project', 'version')
self.build_env.project = self.project
self.build_env.version = self.version

BaseSphinx.type = 'base'
BaseSphinx.sphinx_build_dir = tempfile.mkdtemp()
self.base_sphinx = BaseSphinx(build_env=build_env, python_env=None)

@patch(
'readthedocs.doc_builder.backends.sphinx.SPHINX_TEMPLATE_DIR',
Expand All @@ -47,7 +47,8 @@ def setUp(self):
@patch('readthedocs.doc_builder.backends.sphinx.BaseSphinx.run')
@patch('readthedocs.builds.models.Version.get_conf_py_path')
@patch('readthedocs.builds.models.Project.conf_file')
def test_create_conf_py(self, conf_file, get_conf_py_path, _, get_config_params, create_index, docs_dir):
@patch('readthedocs.projects.models.Project.checkout_path')
def test_create_conf_py(self, checkout_path, conf_file, get_conf_py_path, _, get_config_params, create_index, docs_dir):
"""
Test for a project without ``conf.py`` file.

Expand All @@ -59,20 +60,35 @@ def test_create_conf_py(self, conf_file, get_conf_py_path, _, get_config_params,
any kind of exception raised by ``append_conf`` (we were originally
having a ``TypeError`` because of an encoding problem in Python3)
"""
checkout_path.return_value = tempfile.mkdtemp()
docs_dir.return_value = tempfile.mkdtemp()
create_index.return_value = 'README.rst'
get_config_params.return_value = {}
get_conf_py_path.side_effect = ProjectConfigurationError
conf_file.return_value = tempfile.mktemp()
python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
config=None,
)
base_sphinx = BaseSphinx(
build_env=self.build_env,
python_env=python_env,
)
try:
self.base_sphinx.append_conf()
base_sphinx.append_conf()
except Exception:
pytest.fail('Exception was generated when append_conf called.')

# Check the content generated by our method is the same than what we
# expects from a pre-generated file
generated_conf_py = os.path.join(self.base_sphinx.docs_dir(), 'conf.py')
expected_conf_py = os.path.join(os.path.dirname(__file__), '..', 'files', 'conf.py')
generated_conf_py = os.path.join(base_sphinx.docs_dir(), 'conf.py')
expected_conf_py = os.path.join(
os.path.dirname(__file__),
'..',
'files',
'conf.py'
)
with open(generated_conf_py) as gf, open(expected_conf_py) as ef:
self.assertEqual(gf.read(), ef.read())

Expand All @@ -95,9 +111,14 @@ def test_append_conf_create_yaml(self, checkout_path, run):
os.mkdir(os.path.join(tmpdir, 'docs'))
checkout_path.return_value = tmpdir

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
config=None,
)
self.searchbuilder = MkdocsHTML(
build_env=self.build_env,
python_env=None
python_env=python_env,
)
self.searchbuilder.append_conf()

Expand Down Expand Up @@ -150,9 +171,14 @@ def test_append_conf_existing_yaml_on_root(self, checkout_path, run):
)
checkout_path.return_value = tmpdir

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
config=None,
)
self.searchbuilder = MkdocsHTML(
build_env=self.build_env,
python_env=None
python_env=python_env,
)
self.searchbuilder.append_conf()

Expand Down Expand Up @@ -204,9 +230,14 @@ def test_override_theme_new_style(self, checkout_path, run):
)
checkout_path.return_value = tmpdir

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
config=None,
)
self.searchbuilder = MkdocsHTML(
build_env=self.build_env,
python_env=None
python_env=python_env,
)
self.searchbuilder.append_conf()

Expand Down Expand Up @@ -237,9 +268,14 @@ def test_override_theme_old_style(self, checkout_path, run):
)
checkout_path.return_value = tmpdir

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
config=None,
)
self.searchbuilder = MkdocsHTML(
build_env=self.build_env,
python_env=None
python_env=python_env,
)
self.searchbuilder.append_conf()

Expand Down Expand Up @@ -268,9 +304,14 @@ def test_dont_override_theme(self, checkout_path, run):
)
checkout_path.return_value = tmpdir

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
config=None,
)
self.searchbuilder = MkdocsHTML(
build_env=self.build_env,
python_env=None
python_env=python_env,
)
self.searchbuilder.append_conf()

Expand Down