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

Build: remove append_conf _magic_ from MkDocs #11206

Merged
merged 8 commits into from
Apr 16, 2024
Merged

Build: remove append_conf _magic_ from MkDocs #11206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f4a1442
Build: remove `append_conf` _magic_ from MkDocs
humitos Mar 11, 2024
e7cbf0e
Enable Addons if project is MkDocs
humitos Mar 11, 2024
a72ca02
Listen to Version instead
humitos Mar 11, 2024
2260f26
Typo
humitos Mar 11, 2024
90cca93
Update readthedocs/projects/signals.py
humitos Mar 11, 2024
8d78e5c
Remove datetime restriction
humitos Mar 19, 2024
8440aab
Merge branch 'main' into humitos/remove-append-conf-mkdocs
humitos Mar 19, 2024
6811075
Merge branch 'main' into humitos/remove-append-conf-mkdocs
humitos Apr 15, 2024
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
11 changes: 1 addition & 10 deletions readthedocs/config/tests/test_yaml_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
from readthedocs.doc_builder.backends.mkdocs import (
ProxyPythonName,
yaml_dump_safely,
yaml_load_safely,
)
from readthedocs.doc_builder.backends.mkdocs import ProxyPythonName, yaml_load_safely
Copy link

Choose a reason for hiding this comment

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

readthedocs/config/tests/test_yaml_loader.py


content = """
int: 3
Expand Down Expand Up @@ -30,8 +26,3 @@ def test_yaml_load_safely():
assert type(data["other_function"]) is ProxyPythonName
assert data["function"].value == "python_function"
assert data["other_function"].value == "module.other.function"


def test_yaml_dump_safely():
data = yaml_load_safely(content)
assert yaml_load_safely(yaml_dump_safely(data)) == data
246 changes: 15 additions & 231 deletions readthedocs/doc_builder/backends/mkdocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,10 @@
import structlog
import yaml
from django.conf import settings
from django.template import loader as template_loader

from readthedocs.core.utils.filesystem import safe_open
from readthedocs.doc_builder.base import BaseBuilder
from readthedocs.doc_builder.exceptions import MkDocsYAMLParseError
from readthedocs.projects.constants import MKDOCS, MKDOCS_HTML
from readthedocs.projects.exceptions import UserFileNotFound
from readthedocs.projects.models import Feature

log = structlog.get_logger(__name__)

Expand Down Expand Up @@ -49,31 +45,23 @@ def __init__(self, *args, **kwargs):
# This is the *MkDocs* yaml file
self.yaml_file = self.get_yaml_config()

# README: historically, the default theme was ``readthedocs`` but in
# https://github.com/rtfd/readthedocs.org/pull/4556 we change it to
# ``mkdocs`` to maintain the same behavior in Read the Docs than
# building locally. Although, we can't apply this into the Corporate
# site. To keep the same default theme there, we created a Feature flag
# for these project that were building with MkDocs in the Corporate
# site.
if self.project.has_feature(Feature.MKDOCS_THEME_RTD):
self.DEFAULT_THEME_NAME = "readthedocs"
log.warning(
"Project using readthedocs theme as default for MkDocs.",
project_slug=self.project.slug,
)
else:
self.DEFAULT_THEME_NAME = "mkdocs"

def get_final_doctype(self):
"""
Select a doctype based on the ``use_directory_urls`` setting.

https://www.mkdocs.org/user-guide/configuration/#use_directory_urls
"""

# TODO: we should eventually remove this method completely and stop
# relying on "loading the `mkdocs.yml` file in a safe way just to know
# if it's a MKDOCS or MKDOCS_HTML documentation type".

# Allow symlinks, but only the ones that resolve inside the base directory.
with safe_open(
self.yaml_file, "r", allow_symlinks=True, base_path=self.project_path
self.yaml_file,
"r",
allow_symlinks=True,
base_path=self.project_path,
) as fh:
config = yaml_load_safely(fh)
use_directory_urls = config.get("use_directory_urls", True)
Expand All @@ -89,192 +77,23 @@ def get_yaml_config(self):
mkdocs_path,
)

def load_yaml_config(self):
"""
Load a YAML config.

:raises: ``MkDocsYAMLParseError`` if failed due to syntax errors.
"""
try:
# Allow symlinks, but only the ones that resolve inside the base directory.
result = safe_open(
self.yaml_file, "r", allow_symlinks=True, base_path=self.project_path
)
if not result:
raise UserFileNotFound(
message_id=UserFileNotFound.FILE_NOT_FOUND,
format_values={
"filename": self.yaml_file,
},
)

config = yaml_load_safely(result)

if not config:
raise MkDocsYAMLParseError(MkDocsYAMLParseError.EMPTY_CONFIG)
if not isinstance(config, dict):
raise MkDocsYAMLParseError(MkDocsYAMLParseError.CONFIG_NOT_DICT)
return config

except IOError:
raise MkDocsYAMLParseError(MkDocsYAMLParseError.NOT_FOUND)
except yaml.YAMLError as exc:
note = ""
if hasattr(exc, "problem_mark"):
mark = exc.problem_mark
note = " (line %d, column %d)" % (
mark.line + 1,
mark.column + 1,
)
raise MkDocsYAMLParseError(
MkDocsYAMLParseError.SYNTAX_ERROR,
) from exc

def append_conf(self):
"""
Set mkdocs config values.
Call `cat mkdocs.yaml` only.

:raises: ``MkDocsYAMLParseError`` if failed due to known type errors
(i.e. expecting a list and a string is found).
"""
user_config = self.load_yaml_config()

# Handle custom docs dirs
docs_dir = user_config.get("docs_dir", "docs")
if not isinstance(docs_dir, (type(None), str)):
raise MkDocsYAMLParseError(
MkDocsYAMLParseError.INVALID_DOCS_DIR_CONFIG,
)

user_config["docs_dir"] = docs_dir
static_url = self.project.proxied_static_path

# Set mkdocs config values.
extra_assets = {
"extra_javascript": [
"readthedocs-data.js",
f"{static_url}core/js/readthedocs-doc-embed.js",
f"{static_url}javascript/readthedocs-analytics.js",
],
"extra_css": [
f"{static_url}css/badge_only.css",
f"{static_url}css/readthedocs-doc-embed.css",
],
}

for config, extras in extra_assets.items():
value = user_config.get(config, [])
if value is None:
value = []
if not isinstance(value, list):
raise MkDocsYAMLParseError(
message_id=MkDocsYAMLParseError.INVALID_EXTRA_CONFIG,
format_values={
"extra_config": config,
},
)
# Add the static file only if isn't already in the list.
value.extend([extra for extra in extras if extra not in value])
user_config[config] = value

# The docs path is relative to the location
# of the mkdocs configuration file.
docs_path = os.path.join(
os.path.dirname(self.yaml_file),
docs_dir,
)

# if user puts an invalid `docs_dir` path raise an Exception
if not os.path.exists(docs_path):
raise MkDocsYAMLParseError(
MkDocsYAMLParseError.INVALID_DOCS_DIR_PATH,
)

# RTD javascript writing
rtd_data = self.generate_rtd_data(
docs_dir=os.path.relpath(docs_path, self.project_path),
mkdocs_config=user_config,
)
with safe_open(
os.path.join(docs_path, "readthedocs-data.js"), "w", encoding="utf-8"
) as f:
f.write(rtd_data)

# Use Read the Docs' analytics setup rather than mkdocs'
# This supports using RTD's privacy improvements around analytics
user_config["google_analytics"] = None

# README: make MkDocs to use ``readthedocs`` theme as default if the
# user didn't specify a specific theme manually
if self.project.has_feature(Feature.MKDOCS_THEME_RTD):
if "theme" not in user_config:
# mkdocs<0.17 syntax
user_config["theme"] = self.DEFAULT_THEME_NAME

# Write the modified mkdocs configuration
with safe_open(self.yaml_file, "w", encoding="utf-8") as f:
yaml_dump_safely(
user_config,
f,
)
This behavior has changed. We used to parse the YAML file and append
some configs automatically, but we have been removing that magic from
our builders as much as we can.

This method will eventually removed completely.
"""
# Write the mkdocs.yml to the build logs
self.run(
"cat",
os.path.relpath(self.yaml_file, self.project_path),
cwd=self.project_path,
)

def generate_rtd_data(self, docs_dir, mkdocs_config):
"""Generate template properties and render readthedocs-data.js."""
# Use the analytics code from mkdocs.yml
# if it isn't set already by Read the Docs,
analytics_code = self.version.project.analytics_code
if not analytics_code and mkdocs_config.get("google_analytics"):
# http://www.mkdocs.org/user-guide/configuration/#google_analytics
analytics_code = mkdocs_config["google_analytics"][0]

commit = (
self.version.project.vcs_repo(
version=self.version.slug,
environment=self.build_env,
).commit,
)

# Will be available in the JavaScript as READTHEDOCS_DATA.
readthedocs_data = {
"project": self.version.project.slug,
"version": self.version.slug,
"language": self.version.project.language,
"programming_language": self.version.project.programming_language,
"page": None,
"theme": self.get_theme_name(mkdocs_config),
"builder": "mkdocs",
"docroot": docs_dir,
"source_suffix": ".md",
"api_host": settings.PUBLIC_API_URL,
"ad_free": not self.project.show_advertising,
"commit": commit,
"global_analytics_code": (
None
if self.project.analytics_disabled
else settings.GLOBAL_ANALYTICS_CODE
),
"user_analytics_code": analytics_code,
"proxied_static_path": self.project.proxied_static_path,
"proxied_api_host": self.project.proxied_api_host,
}

data_ctx = {
"readthedocs_data": readthedocs_data,
"current_version": readthedocs_data["version"],
"slug": readthedocs_data["project"],
"html_theme": readthedocs_data["theme"],
"pagename": None,
}
tmpl = template_loader.get_template("doc_builder/data.js.tmpl")
return tmpl.render(data_ctx)

def build(self):
build_command = [
self.python_env.venv_bin(filename="python"),
Expand All @@ -296,42 +115,12 @@ def build(self):
)
return cmd_ret.successful

def get_theme_name(self, mkdocs_config):
"""
Get the theme configuration in the mkdocs_config.

In v0.17.0, the theme configuration switched
from two separate configs (both optional) to a nested directive.

:see: http://www.mkdocs.org/about/release-notes/#theme-customization-1164
:returns: the name of the theme RTD will use
"""
theme_setting = mkdocs_config.get("theme")
if isinstance(theme_setting, dict):
# Full nested theme config (the new configuration)
return theme_setting.get("name") or self.DEFAULT_THEME_NAME

if theme_setting:
# A string which is the name of the theme
return theme_setting

theme_dir = mkdocs_config.get("theme_dir")
if theme_dir:
# Use the name of the directory in this project's custom theme directory
return theme_dir.rstrip("/").split("/")[-1]

return self.DEFAULT_THEME_NAME


class MkdocsHTML(BaseMkdocs):
builder = "build"
build_dir = "_readthedocs/html"


# TODO: find a better way to integrate with MkDocs.
# See https://github.com/readthedocs/readthedocs.org/issues/7844


class ProxyPythonName(yaml.YAMLObject):
def __init__(self, value):
self.value = value
Expand Down Expand Up @@ -389,8 +178,3 @@ def yaml_load_safely(content):
information loss.
"""
return yaml.load(content, Loader=SafeLoader)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove all code related to SafeLoader and just call safe_load now.

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 can't yet because it's used to get the final doctype for the version at

readthedocs.org/readthedocs/doc_builder/backends/mkdocs.py

Lines 66 to 68 in 8440aab

config = yaml_load_safely(fh)
use_directory_urls = config.get("use_directory_urls", True)
return MKDOCS if use_directory_urls else MKDOCS_HTML

This code will be eventually removed since the doctype doesn't have too much sense anymore now that we support many doctools more. We should probably need to have a conversation about how to delete doctypes completely in a safe way.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, but don't think you need the custom loader to just read that setting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm, but without the safeloader we will executing !!python syntax, which is why we added it, right?

Copy link
Member

Choose a reason for hiding this comment

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

No if you call safe_load()

Copy link
Member

Choose a reason for hiding this comment

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

The custom loader was added to don't set those values to None after reading them (we were using safe_load previously).

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool. Good point! I will delete this chunk of code then 👍🏼



def yaml_dump_safely(content, stream=None):
"""Uses ``SafeDumper`` dumper to write YAML contents."""
return yaml.dump(content, stream=stream, Dumper=SafeDumper)
1 change: 1 addition & 0 deletions readthedocs/projects/apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class ProjectsConfig(AppConfig):
def ready(self):
# Load and register notification messages for this application
import readthedocs.projects.notifications # noqa
import readthedocs.projects.signals # noqa
import readthedocs.projects.tasks.builds # noqa
import readthedocs.projects.tasks.search # noqa
import readthedocs.projects.tasks.utils # noqa
5 changes: 0 additions & 5 deletions readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1877,7 +1877,6 @@ def add_features(sender, **kwargs):

# Feature constants - this is not a exhaustive list of features, features
# may be added by other packages
MKDOCS_THEME_RTD = "mkdocs_theme_rtd"
API_LARGE_DATA = "api_large_data"
CONDA_APPEND_CORE_REQUIREMENTS = "conda_append_core_requirements"
ALL_VERSIONS_IN_HTML_CONTEXT = "all_versions_in_html_context"
Expand Down Expand Up @@ -1908,10 +1907,6 @@ def add_features(sender, **kwargs):
SCALE_IN_PROTECTION = "scale_in_prtection"

FEATURES = (
(
MKDOCS_THEME_RTD,
_("MkDocs: Use Read the Docs theme for MkDocs as default theme"),
),
(
API_LARGE_DATA,
_("Build: Try alternative method of posting large data"),
Expand Down
Loading