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 all 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
16 changes: 14 additions & 2 deletions readthedocs/config/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,17 @@ def build(self):
def doctype(self):
return self.defaults['doctype']

@property
def sphinx(self):
config_file = self.defaults['sphinx_configuration']
if config_file is not None:
config_file = os.path.join(self.base_path, config_file)
return Sphinx(
builder=self.doctype,
configuration=config_file,
fail_on_warning=False,
)


class BuildConfigV2(BuildConfigBase):

Expand Down Expand Up @@ -959,7 +970,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',
)
32 changes: 21 additions & 11 deletions readthedocs/doc_builder/backends/sphinx.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,14 @@ class BaseSphinx(BaseBuilder):

def __init__(self, *args, **kwargs):
super(BaseSphinx, self).__init__(*args, **kwargs)
self.config_file = self.config.sphinx.configuration
try:
if not self.config_file:
self.config_file = self.project.conf_file(self.version.slug)
self.old_artifact_path = os.path.join(
self.project.conf_dir(self.version.slug), self.sphinx_build_dir)
os.path.dirname(self.config_file),
self.sphinx_build_dir
)
except ProjectConfigurationError:
docs_dir = self.docs_dir()
self.old_artifact_path = os.path.join(
Expand All @@ -69,7 +74,7 @@ def get_config_params(self):
# TODO this should be handled better in the theme
conf_py_path = os.path.join(
os.path.sep,
self.version.get_conf_py_path(),
self.config_file,
'',
)
remote_version = self.version.commit_name
Expand Down Expand Up @@ -151,15 +156,16 @@ def get_config_params(self):

def append_conf(self, **__):
"""Find or create a ``conf.py`` with a rendered ``doc_builder/conf.py.tmpl`` appended"""
try:
self.version.get_conf_py_path()
except ProjectConfigurationError:
if self.config_file is None:
master_doc = self.create_index(extension='rst')
self._write_config(master_doc=master_doc)

try:
outfile_path = self.project.conf_file(self.version.slug)
outfile = codecs.open(outfile_path, encoding='utf-8', mode='a')
self.config_file = (
self.config_file or
self.project.conf_file(self.version.slug)
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 need to keep this one, as here we catch and re-raise the exception to the user

)
outfile = codecs.open(self.config_file, encoding='utf-8', mode='a')
except (ProjectConfigurationError, IOError):
trace = sys.exc_info()[2]
six.reraise(
Expand All @@ -183,7 +189,7 @@ def append_conf(self, **__):
self.run(
'cat',
os.path.relpath(
outfile_path,
self.config_file,
self.project.checkout_path(self.version.slug),
),
cwd=self.project.checkout_path(self.version.slug),
Expand All @@ -199,6 +205,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 @@ -210,8 +218,10 @@ def build(self):
self.sphinx_build_dir,
])
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(self.config_file),
bin_path=self.python_env.venv_bin()
)
return cmd_ret.successful


Expand Down Expand Up @@ -347,7 +357,7 @@ class PdfBuilder(BaseSphinx):

def build(self):
self.clean()
cwd = self.project.conf_dir(self.version.slug)
cwd = os.path.dirname(self.config_file)

# Default to this so we can return it always.
self.run(
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=update_docs.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
Loading