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.

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

Implement sphinx key from v2 config #4482

Merged
merged 21 commits into from
Sep 7, 2018
Merged

Implement sphinx key from v2 config #4482

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f7dc3bd
Test conf.py default value
stsewd Aug 3, 2018
09ea0dc
Fix conf.py path
stsewd Aug 6, 2018
0648f65
Don't skip tests
stsewd Aug 6, 2018
e9db42c
Expose config object in builders
stsewd Aug 6, 2018
7fe49ee
Use doctype from config
stsewd Aug 6, 2018
954e277
Pass python_env in tests
stsewd Aug 6, 2018
292f912
Fix old test
stsewd Aug 6, 2018
19810f4
Implement sphinx key
stsewd Aug 6, 2018
fb48f7c
Implement fail_on_warning
stsewd Aug 6, 2018
1083999
Skip tests for mkdocs
stsewd Aug 6, 2018
d35c23c
Fix raising wrong exception
stsewd Aug 6, 2018
58adc35
Fix tests
stsewd Aug 7, 2018
819a9fa
Remove search from tests
stsewd Aug 25, 2018
e6b9dd5
Return the absolute path
stsewd Aug 27, 2018
db40aa7
Check for None
stsewd Aug 27, 2018
b4c22dd
Use the config file
stsewd Aug 27, 2018
6ee2d66
Fix test
stsewd Aug 27, 2018
4750efd
Merge branch 'master' into build-config-v2-phinx
stsewd Aug 27, 2018
aa25732
Fix test
stsewd Aug 27, 2018
189726c
Remove repetition
stsewd Aug 29, 2018
dc99eb8
Break and fix test
stsewd Aug 29, 2018
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
View file
Open in desktop
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',
)
33 changes: 24 additions & 9 deletions readthedocs/doc_builder/backends/sphinx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,12 @@ class BaseSphinx(BaseBuilder):
def __init__(self, *args, **kwargs):
super(BaseSphinx, self).__init__(*args, **kwargs)
try:
config_file = self.config.sphinx.configuration
config_dir = os.path.dirname(config_file) if config_file else None
self.old_artifact_path = os.path.join(
self.project.conf_dir(self.version.slug), self.sphinx_build_dir)
config_dir or self.project.conf_dir(self.version.slug),
self.sphinx_build_dir
)
except ProjectConfigurationError:
docs_dir = self.docs_dir()
self.old_artifact_path = os.path.join(
Expand All @@ -69,7 +73,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.sphinx.configuration or self.version.get_conf_py_path(),
'',
)
remote_version = self.version.commit_name
Expand Down Expand Up @@ -151,14 +155,15 @@ 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.sphinx.configuration 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_path = (
self.config.sphinx.configuration 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

)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems here we're implying that if you set a sphinx configuration file, but it doesn't exist, we just create it? I feel like we should error out if you set up a config file and it doesn't actually exist, it will be confusing to the user for the build to pass in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to keep compatibility with the behavior when the user doesn't set a config file and rtd tries to find one.

Copy link
Contributor

Choose a reason for hiding this comment

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

That part I understand, but this seemed to be creating a 3rd option, where the config was specified but not found, and therefore the config was generated instead. This might not be an issue, I interpreted self.config.sphinx.configuration as defaulting to None if the file is specified but doesn't exist, but that doesn't seem to be the logic used looking at it again.

outfile = codecs.open(outfile_path, encoding='utf-8', mode='a')
except (ProjectConfigurationError, IOError):
trace = sys.exc_info()[2]
Expand Down Expand Up @@ -199,6 +204,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 +216,15 @@ def build(self):
'.',
self.sphinx_build_dir,
])
config_file = (
self.config.sphinx.configuration or
self.project.conf_file(self.version.slug)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. And I feel like we shouldn't be guessing here. If the config file was specified and it exists, that's when we should set the config file path. Likewise, if we're generating a config, we set a config file path when we find out the user doesn't have a config and didn't specify a config file path.

This will cut down on magical decisions that tend to hide errors from us.

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 Expand Up @@ -347,7 +360,9 @@ class PdfBuilder(BaseSphinx):

def build(self):
self.clean()
cwd = self.project.conf_dir(self.version.slug)
config_file = self.config.sphinx.configuration
config_dir = os.path.dirname(config_file) if config_file else None
cwd = config_dir or self.project.conf_dir(self.version.slug)
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic looks duplicated, is there a way to reduce this duplication if so?

Copy link
Member Author

@stsewd stsewd Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I knew you were not going to like it p: haha, so all this is to make sure that we are keeping the previous behavior. If a config file doesn't exist, we create one, and also we try to find one in the whole project. I guess we can do a big refactor to do this step early, before parsing the config file (but not sure if this worth it). Or maybe we can use a config_file property inside the sphinx builder and modify that as we want

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this could either be a property, or even just simply set the common values on __init__ in base?


# Default to this so we can return it always.
self.run(
Expand Down
1 change: 1 addition & 0 deletions readthedocs/doc_builder/base.py
View file
Open in desktop
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
View file
Open in desktop
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.

We didn't have a test, but I added them in https://github.com/rtfd/readthedocs.org/pull/4456/files#diff-5a2cc4147895068b33fe5bf89e4e6e08R589 (skipped)

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
View file
Open in desktop
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
View file
Open in desktop
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