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

mkdocs: Values with special ! syntax are set to null on mkdocs.yml #8529

Closed
j1elo opened this issue Sep 27, 2021 · 14 comments · Fixed by #11206
Closed

mkdocs: Values with special ! syntax are set to null on mkdocs.yml #8529

j1elo opened this issue Sep 27, 2021 · 14 comments · Fixed by #11206
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code Needed: documentation Documentation is required

Comments

@j1elo
Copy link

j1elo commented Sep 27, 2021

Details

Our project is using:

Expected Result

  1. The cat mkdocs.yml build step should show proper values for env vars READTHEDOCS, READTHEDOCS_VERSION, and READTHEDOCS_PROJECT.

  2. A test html file with these contents:

    console.log("config.extra: {{ config.extra }}");
    console.log("config.extra.READTHEDOCS: {{ config.extra.READTHEDOCS }}");
    console.log("config.extra.READTHEDOCS_VERSION: {{ config.extra.READTHEDOCS_VERSION }}");
    

    should print something similar to this in the browser console:

    config.extra: {'READTHEDOCS': True, 'READTHEDOCS_VERSION': 1.2.3, 'READTHEDOCS_PROJECT': openvidu}
    config.extra.READTHEDOCS: True
    config.extra.READTHEDOCS_VERSION: 1.2.3
    

Actual Result

  1. The cat mkdocs.yml build step shows that READTHEDOCS, READTHEDOCS_VERSION, and READTHEDOCS_PROJECT are all null:

    extra:
      READTHEDOCS: null
      READTHEDOCS_PROJECT: null
      READTHEDOCS_VERSION: null
    
  2. The browser console shows this output:

    config.extra: {'READTHEDOCS': None, 'READTHEDOCS_PROJECT': None, 'READTHEDOCS_VERSION': None}
    config.extra.READTHEDOCS: None
    config.extra.READTHEDOCS_VERSION: None
    

Extra

Question / potential extra bug in the documentation: the technique shown in How do I change behavior when building with Read the Docs? would not really work with MkDocs, would it?

The sample is:

{% if READTHEDOCS %}
Woo
{% endif %}

but in MkDocs the READTHEDOCS is not directly available, is it expected to be? (sorry to ask here but is just a yes/no question that seems very relevant given the topic of the issue)

@humitos
Copy link
Member

humitos commented Sep 28, 2021

with the ENV syntax to read environment variables, making the READTHEDOCS vars available to the template context:

Reading that documentation, I arrived at https://github.com/waylan/pyyaml-env-tag#enabling-the-tag. It doesn't work on Read the Docs because we are using our own YAML parser for security reasons (

class SafeLoader(yaml.SafeLoader): # pylint: disable=too-many-ancestors
"""
Safe YAML loader.
This loader parses special ``!!python/name:`` tags without actually
importing or executing code. Every other special tag is ignored.
Borrowed from https://stackoverflow.com/a/57121993
Issue https://github.com/readthedocs/readthedocs.org/issues/7461
"""
def ignore_unknown(self, node): # pylint: disable=no-self-use, unused-argument
return None
def construct_python_name(self, suffix, node): # pylint: disable=no-self-use, unused-argument
return ProxyPythonName(suffix)
) and the !ENV syntax is not recognized.

Unfortunately, I don't think there is a workaround, currently.

This may be solved by our "Future builders" (#8190) because we may not require a safe YAML loader when it gets implemented.

@j1elo
Copy link
Author

j1elo commented Sep 28, 2021

Thank you for the feedback. So far I've been left with the only choice of loading the (otherwise undocumented!) script readthedocs-data.js, which is auto-generated by RTD MkDocs backend. This file exports a JS object with some values that I can use at run time, although I'd have preferred to have them at template build time.

This bring the next followup question, not sure if worth a separate bug report: why the RTD context injection is not working either? I was hoping to use that as alternative to the env vars, but the values don't resolve either and the build fails.

Seems to me that the general state of MkDocs support in RTD is pretty weak, as soon as you try to do anything beyond the most basic use cases; no problem with that, but I do believe that adding appropriate comments in some of these doc pages would save long hours of frustration to users (at least I know it would have for me...)

@humitos
Copy link
Member

humitos commented Sep 28, 2021

Hi @j1elo! Thanks for replying. You have mentioned here some interesting points that have been in our discussions lately but we haven't yet had the time to fix them.

Thank you for the feedback. So far I've been left with the only choice of loading the (otherwise undocumented!) script readthedocs-data.js

This is not documented on purpose because we don't expect users to use this data as it may change without communication. I don't think it has changed in years now. However, we can't compromise ourselves on that.

This bring the next followup question, not sure if worth a separate bug report: why the RTD context injection is not working either?

All the pages under /development/design/ are documents that we used to discuss proposed features. Some of them were never implemented --as the Theme Context that you just found 😞 . We have discussed not exposing these pages to users or moving them to a different set of documentation because we have already faced the confusion you are experimenting.

Seems to me that the general state of MkDocs support in RTD is pretty weak, as soon as you try to do anything beyond the most basic use cases

Unfortunately, this is true. Due to security restrictions, there are advanced MkDocs configurations that we cannot support currently and we haven't had the time to change our internal implementation to reduce those risks. For the !ENV case, we could use a similar solution than for !! in #7461

no problem with that, but I do believe that adding appropriate comments in some of these doc pages would save long hours of frustration to users (at least I know it would have for me...)

I'm sorry to read that. I hope we can clarify these limitations in our documentation. @astrojuanlu what do you think about mentioning that non-standard (or advanced) YAML usage is not supported currently?

@humitos humitos added Improvement Minor improvement to code Needed: documentation Documentation is required labels Sep 28, 2021
@j1elo
Copy link
Author

j1elo commented Sep 28, 2021

Thank you very much for the comprehensive answer! :)

Thank you for the feedback. So far I've been left with the only choice of loading the (otherwise undocumented!) script readthedocs-data.js

This is not documented on purpose because we don't expect users to use this data as it may change without communication. I don't think it has changed in years now. However, we can't compromise ourselves on that.

At the end of the day what I'm trying to do is make a small decision (to set <base href='/<Language>/<Version>/'> to the correct values in my HTML output... I know that's not ideal, but it's what I got in my hands right now), depending on the version that is being built. For that, my route was to try the Theme Context feature, then the Environment Variables, and lastly I ended up finding the existence of this script.

Are there any more alternatives that I could try with MkDocs? Otherwise, I'm willing to make the compromise of using a private RTD feature that is subject to change.

@astrojuanlu
Copy link
Contributor

All the pages under /development/design/ are documents that we used to discuss proposed features. Some of them were never implemented --as the Theme Context that you just found 😞 . We have discussed not exposing these pages to users or moving them to a different set of documentation because we have already faced the confusion you are experimenting.

Do we have an issue for that already? Or are we only tracking it internally?

Besides, I think that design doc in particular has a confusing prose. It talks to "you" (the user) in a way a tutorial would do, and besides there is an admonition on top of https://docs.readthedocs.io/en/stable/guides/vcs.html that leads users into thinking that the new vision is already in place. I'll address this separately.

@j1elo
Copy link
Author

j1elo commented Sep 28, 2021

Note for my future self or anyone with a similar need: Editing the <base> element after the fact with a JavaScript block (as was my intention here) is a very finicky thing to do. Resources will load first with the default base value, mostly giving 404s, then reload again with the modified base. So the only really decent solution is that <base href="..."> is already set correctly to start with, done from the templating engine.

This is my current solution. Very hacky, very ugly, very brittle. But that's the only thing that has worked so far. Only tested with the latest version...

<head>
<meta charset="utf-8">

<!--
Create a `<base>` element that matches the current RTD version.
There are 2 parts of the URL slug we need to get:
* Language.
* Version name. Something like "latest", "stable", "2.20.0", etc.
-->
{%- if config.extra.READTHEDOCS %}
    <!--
    This would be a nicer method to get the values. Load the RTD env vars into the MkDocs config,
    which itself gets passed by MkDocs into the Jinja's template context.
    However as of this writing, this method won't work.
    -->
    <base href="/{{ config.extra.READTHEDOCS_LANGUAGE }}/{{ config.extra.READTHEDOCS_VERSION }}/" />
{%- elif "doc.example.org" in config.site_url %}
    <!--
    Ugly Hack:
    * Language: Obtained from the theme's locale config.
    * Version name: RTD checks out the repo into a directory named like the
    version that is being built. So we get the version name from there.
    Sample `docs_dir` string for a build of the `latest` version:
    "/home/docs/checkouts/readthedocs.org/user_builds/openvidu/checkouts/latest/docs".
    -->
    <base href="/{{ config.theme.locale.language }}/{{ config.docs_dir.split('/')[-2] }}/" />
{%- else %}
    <!--
    When not deploying in our production subdomain, default to root.
    This is the case when using `mkdocs serve` for local authoring.
    -->
    <base href="/" />
{%- endif %}

Needless to say, it would be really nice if RTD ends up adding some of its build information to the context that gets passed to Jinja. Meanwhile, I sincerely wasn't able to find any solution that doesn't obtain the info in such indirect way.

Regardless, thank you all very much for your help and your support

@stsewd stsewd changed the title READTHEDOCS environment variables are null mkdocs: READTHEDOCS environment variables are null on mkdocs.yml Sep 29, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Mar 2, 2022
@humitos
Copy link
Member

humitos commented Mar 2, 2022

I hope we can clarify these limitations in our documentation. @astrojuanlu what do you think about mentioning that non-standard (or advanced) YAML usage is not supported currently?

It seems this is the only actionable item from this issue for now. I'm marking it as accepted.

@stale stale bot removed the Status: stale Issue will be considered inactive soon label Mar 2, 2022
@humitos humitos added Accepted Accepted issue on our roadmap Status: stale Issue will be considered inactive soon labels Mar 2, 2022
@stale stale bot removed the Status: stale Issue will be considered inactive soon label Mar 2, 2022
@j1elo
Copy link
Author

j1elo commented Mar 4, 2022

Hi,

It seems this is the only actionable item from this issue for now.

I guess an additional one was suggested from these sentences, but was never stated as a proper actionable item:

Which I'd take to become either of:

  • !ENV syntax support addition.
  • Workaround provided for usage of such syntax.
  • Decision taken about whether the "may" is to become a "will" in the phrase may be solved by our "Future builders" (this sounds to me like the best option, if those Future builders are actually going to become a reality at some point)
  • Follow the trail of a "similar solution than for !!" and see if the same ideas can actually be applied.

@humitos
Copy link
Member

humitos commented Mar 21, 2023

Once #10166 gets merged we will be exposing the canonical URL as READTHEDOCS_CANONICAL_URL environment variable that people can use from MkDocs and any other doctool. However, the normal build process does not support !! ENV on the mkdocs.yml file, so users won't be able to use this variable in the "MkDocs way" and may need to implement some small tricks.

Due to that, I see two possible solutions here:

  1. implement support for !! ENV and handle it in the same way as we are doing for !1python (see Special !! values are reset to null in mkdocs.yml #7461)
  2. tell these users to override the full build process by using build.commands (see https://docs.readthedocs.io/en/stable/build-customization.html) so they can use the !! ENV syntax

I think that 1) should definitely happens at some time when we can prioritize, but at least 2) is a good workaround for now and unblock users.

FeeeeK added a commit to vkbottle/vkbottle that referenced this issue Apr 17, 2023
lars-reimann added a commit to Safe-DS/Library that referenced this issue Jul 11, 2023
Closes #443

### Summary of Changes

The 404 page was previously not properly rendered on Read the Docs
(RTD). This PR overrides the build commands of RTD. While this makes the
404 page work properly, it also removes the RTD flyout to select a
version. For now, however, we deem the 404 page as more essential.

Once readthedocs/readthedocs.org#8529 is done,
we can use the default build again to make everything work as expected.
lars-reimann added a commit to Safe-DS/Stub-Generator that referenced this issue Jul 11, 2023
### Summary of Changes

The 404 page was previously not properly rendered on Read the Docs
(RTD). This PR overrides the build commands of RTD. While this makes the
404 page work properly, it also removes the RTD flyout to select a
version. For now, however, we deem the 404 page as more essential.

Once readthedocs/readthedocs.org#8529 is done,
we can use the default build again to make everything work as expected.
lars-reimann added a commit to Safe-DS/Library-Analyzer that referenced this issue Jul 11, 2023
### Summary of Changes

The 404 page was previously not properly rendered on Read the Docs
(RTD). This PR overrides the build commands of RTD. While this makes the
404 page work properly, it also removes the RTD flyout to select a
version. For now, however, we deem the 404 page as more essential.

Once readthedocs/readthedocs.org#8529 is done,
we can use the default build again to make everything work as expected.
lars-reimann added a commit to Safe-DS/Datasets that referenced this issue Jul 11, 2023
### Summary of Changes

The 404 page was previously not properly rendered on Read the Docs
(RTD). This PR overrides the build commands of RTD. While this makes the
404 page work properly, it also removes the RTD flyout to select a
version. For now, however, we deem the 404 page as more essential.

Once readthedocs/readthedocs.org#8529 is done,
we can use the default build again to make everything work as expected.
lars-reimann added a commit to Safe-DS/API-Editor that referenced this issue Jul 11, 2023
### Summary of Changes

The 404 page was previously not properly rendered on Read the Docs
(RTD). This PR overrides the build commands of RTD. While this makes the
404 page work properly, it also removes the RTD flyout to select a
version. For now, however, we deem the 404 page as more essential.

Once readthedocs/readthedocs.org#8529 is done,
we can use the default build again to make everything work as expected.

---------

Co-authored-by: megalinter-bot <[email protected]>
lars-reimann added a commit to Safe-DS/DSL that referenced this issue Jul 11, 2023
### Summary of Changes

The 404 page was previously not properly rendered on Read the Docs
(RTD). This PR overrides the build commands of RTD. While this makes the
404 page work properly, it also removes the RTD flyout to select a
version. For now, however, we deem the 404 page as more essential.

Once readthedocs/readthedocs.org#8529 is done,
we can use the default build again to make everything work as expected.

---------

Co-authored-by: megalinter-bot <[email protected]>
@MetalKnight56
Copy link

So on this issue, is there any proper solution to this? I've been struggling trying to use the !ENV argument on my mkdocs.yml ...

@humitos
Copy link
Member

humitos commented Dec 23, 2023

@MetalKnight56 yes, take a look at option 2) from #8529 (comment)

@MetalKnight56
Copy link

@MetalKnight56 yes, take a look at option 2) from #8529 (comment)

Thanks for replying, i was able to fix it on my side using poetry :)

humitos added a commit that referenced this issue Mar 11, 2024
Delete all the magic around MkDocs YAML that processes the `mkdocs.yml`.

file and
define `readthedocs` theme automatically based on a feature flag,
`MKDOCS_THEME_RTD`.

This PR removes:

- automatically defining `readthedocs` as theme when `MKDOCS_THEME_RTD` feature
  flag is defined.
- processing `mkdocs.yml` to add internal JS and CSS (embed and analytics) files
  automatically

This is another step forward on removing all the magic we perform on behalf the
users and being more explicit about how to configure each doctool.

Reference:

* readthedocs/addons#72 (comment)

Closes #8529
humitos added a commit to readthedocs/website that referenced this issue Mar 19, 2024
Blog post announcing the depreaction of the `mkdocs.yml` manipulation done by
Read the Docs at build time, following the plan described.

Related:
* readthedocs/addons#72 (comment)
* readthedocs/readthedocs.org#11206
* readthedocs/readthedocs.org#8529
humitos added a commit to readthedocs/website that referenced this issue Mar 25, 2024
* Post: MkDocs YAML manipulation

Blog post announcing the depreaction of the `mkdocs.yml` manipulation done by
Read the Docs at build time, following the plan described.

Related:
* readthedocs/addons#72 (comment)
* readthedocs/readthedocs.org#11206
* readthedocs/readthedocs.org#8529

* Use double quotes

The other ones don't render properly.

* Apply suggestions from code review

Co-authored-by: Eric Holscher <[email protected]>

---------

Co-authored-by: Eric Holscher <[email protected]>
@humitos
Copy link
Member

humitos commented Apr 8, 2024

Hi 👋🏼 . Starting on April 15th the usage of special ! syntax will be allowed since we are going to stop the manipulation of mkdocs.yml we were doing previously. Read more about this in our blog post at https://about.readthedocs.com/blog/2024/03/mkdocs-yaml-manipulation/

humitos added a commit that referenced this issue Apr 16, 2024
* Build: remove `append_conf` _magic_ from MkDocs

Delete all the magic around MkDocs YAML that processes the `mkdocs.yml`.

file and
define `readthedocs` theme automatically based on a feature flag,
`MKDOCS_THEME_RTD`.

This PR removes:

- automatically defining `readthedocs` as theme when `MKDOCS_THEME_RTD` feature
  flag is defined.
- processing `mkdocs.yml` to add internal JS and CSS (embed and analytics) files
  automatically

This is another step forward on removing all the magic we perform on behalf the
users and being more explicit about how to configure each doctool.

Reference:

* readthedocs/addons#72 (comment)

Closes #8529

* Enable Addons if project is MkDocs

Listen to `Project.post_save` signal and enable Addons if the project was
created after a specific date, it's MkDocs and it's the first time the
`AddonsConfig` is created.

* Listen to Version instead

* Typo

* Update readthedocs/projects/signals.py

Co-authored-by: Santos Gallegos <[email protected]>

* Remove datetime restriction

---------

Co-authored-by: Santos Gallegos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code Needed: documentation Documentation is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants