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 upgrade breaks existing valid projects #4314

Closed
marcelstoer opened this issue Jun 30, 2018 · 14 comments
Closed

MkDocs upgrade breaks existing valid projects #4314

marcelstoer opened this issue Jun 30, 2018 · 14 comments
Assignees

Comments

@marcelstoer
Copy link
Contributor

marcelstoer commented Jun 30, 2018

Details

Expected Result

As the project builds locally just fine with MkDocs 0.17.3 it should build fine on RTD as well.

Actual Result

The build fails with

WARNING -  Config value: 'theme_dir'. Warning: The configuration option {0} has been deprecated and will be removed in a future release of MkDocs. 

Aborted with 1 Configuration Warnings in 'strict' mode!

I double checked my mkdocs.yml but there's no theme_dir anywhere in sight: https://github.com/nodemcu/nodemcu-firmware/blob/dev/mkdocs.yml. However, thanks to the cat mkdocs.yml output you provide for every build (great idea!) I see that the RTD build adds it!

So, if you manipulate our build config on-the-fly then please do it in a way that doesn't break builds.

I'm happy about the MkDocs upgrade, that I found out about by coincidence, thank you 👏. But couldn't you inform affected users/projects by email when you do major upgrades? Any "release" or upgrade channel we can subscribe to?

@davidfischer davidfischer self-assigned this Jun 30, 2018
@davidfischer
Copy link
Contributor

Read the Docs modifies mkdocs.yml in order to inject the version menu where additional versions of the docs are linked and a few other things. See here. We (I) tried to do this in a compatible way in case people had pinned to a previous mkdocs version in their requirements file. However it does create a warning in the latest mkdocs. Because you've specified strict mode in your config, that became an error. As a short term fix to get building again, turn off strict mode.

As a better fix I suppose we will have to detect the version of mkdocs and do the right thing based on the version. The other opetion is to change how we inject that version data.

I'm happy about the MkDocs upgrade, that I found out about by coincidence, thank you 👏. But couldn't you inform affected users/projects by email when you do major upgrades? Any "release" or upgrade channel we can subscribe to?

We don't have an announcements mailing list but I do think that's a good idea. Currently the deployed version number is shown in the footer and links to the changelog.

@marcelstoer
Copy link
Contributor Author

I get the thing about the version menu. However, the warning appears to be about the deprecated theme_dir option. Why does RTD need that at all? Isn't the theme available in the environment under the readthedocs name (making theme_dir superfluous)?

We don't have an announcements mailing list but I do think that's a good idea.

How about a new @readthedocs-release on Twitter to which your release script posts a link to the changelog?

@davidfischer
Copy link
Contributor

Why does RTD need that at all? Isn't the theme available in the environment under the readthedocs name (making theme_dir superfluous)?

We override the default readthedocs theme with a few extra templates. https://github.com/rtfd/readthedocs.org/tree/master/readthedocs/templates/mkdocs/readthedocs

@marcelstoer
Copy link
Contributor Author

Yes, I know. If you install it in the Python environment prior to running mkdocs then you wouldn't need that option at all. At least that's my understanding of how that works. I run self-hosted MkDocs sites with custom themes that work like that.

If installing isn't possible then with v0.17.0 and up

theme_dir: /home/docs/checkouts/readthedocs.org/readthedocs/templates/mkdocs/readthedocs

should be replaced with

theme:
name: readthedocs
custom_dir: /home/docs/checkouts/readthedocs.org/readthedocs/templates/mkdocs/readthedocs

-> https://www.mkdocs.org/about/release-notes/#theme-customization-1164

@davidfischer
Copy link
Contributor

The problem is that the above breaks with anybody who has pinned mkdocs to a version before 0.17.0. I don't see a way around version detection. Do you?

@marcelstoer
Copy link
Contributor Author

marcelstoer commented Jun 30, 2018

Oh, it finally sunk in! You cannot install your theme because it's just a theme customization rather than a full theme.

I don't see a way around version detection. Do you?

Nope, sorry. I don't know how MkDocs is gonna behave once theme_dir (or any other option) isn't just deprecated but not supported anymore. Even the best strict: false won't help anymore. Unless their parser simply skips unknown options it's inevitable that you add version detection.

If it were just about theme options there may be a way around it but my idea is so half-baked that I don't dare to mention it.

Just for the record I'm referencing #4041 and #4302

@davidfischer
Copy link
Contributor

I don't know how MkDocs is gonna behave once theme_dir (or any other option) isn't just deprecated but not supported anymore. Even the best strict: false won't help anymore.

Agreed. We will need a solution by then. I think RTD needs to get a bit smarter in how or even whether versions of sphinx/mkdocs are able to be overridden. This has been the source of a number of problems. Maybe the version used is specified in .readthedocs.yml or something and it can't be overridden by the requirements file.

@TerryE
Copy link

TerryE commented Jul 1, 2018

Hi. I work with Marcel, so have been tracking this. The strict option does have functional advantages for a project in terms of extra checking that most projects should want to do, so this workaround does represent a functional loss at least in terms of QA.

On a separate note, Readthedocs is a great service. Thanks 😄

@davidfischer
Copy link
Contributor

As I mentioned, I view turning off strict as a short term fix to get building again. I think version detection -- that is, detecting the version of mkdocs and doing the right thing -- is a better fix but that will require changes. Do you agree? Do you see another alternative?

@TerryE
Copy link

TerryE commented Jul 2, 2018

Do you agree? Do you see another alternative?

Yes. No. 👍

@JohnStrunk
Copy link

Because I landed here after having my RTD build break w/ strict: true...
Changing the mkdocs.yml:

theme: "readthedocs"

to

theme:
  name: "readthedocs"

fixes the issue (and lets you keep strict)
Ref: gluster/anthill#37

@marcelstoer
Copy link
Contributor Author

@JohnStrunk not sure if this is a catch-all solution. I noticed that on https://nodemcu.readthedocs.io/en/latest/ the fly-out menu bottom left is gone/broken (used the above snippet).

@marcelstoer
Copy link
Contributor Author

marcelstoer commented Aug 21, 2018

Nevermind, it's actually the same issue I noticed already back in January: https://groups.google.com/d/topic/mkdocs/v7AVbeB105w/discussion. It broke when RTD moved the default MkDocs version to >= 0.16 (currently 0.17.3).

  1. I'll report it as a separate issue here -> Fly-out menu broken from MkDocs projects #4549
  2. @davidfischer I'd like to register our NodeMCU project as one of the MkDocs projects you verify the correctness of your changes against (silently assuming something like this exists). How shall I proceed?

@davidfischer
Copy link
Contributor

@marcelstoer I'll definitely check your project going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants