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

Update information on mkdocs build process #4508

Merged
merged 5 commits into from Aug 21, 2018

Conversation

@AumitLeon
Copy link
Contributor

@AumitLeon AumitLeon commented Aug 12, 2018

This PR should resolve #4507. As per #4231, builds fail with mkdocs when markdown files are not in a doc or docs directory. The mkdocs.yml could either be in the root or in the docs directory, as per https://www.mkdocs.org/about/release-notes/#stricter-directory-validation and my own testing within my own project.

I was able to solve this issue by moving my mkdocs.yml and relevant markdown files to a docs directory. I also tested this with a doc directory, see this closed PR in my project. I think it makes sense to have the docs reflect this so future users know to have their files in the correct directory.

Of course, this doesn't solve the issue of builds failing when files are at the root, but users should be aware of the current caveat.

This is my first PR, so I am definitely open to feedback, comments, and questions :)

Copy link
Member

@humitos humitos left a comment

Thanks for your contribution!

First, I'm not an expert on MkDocs. That said, I tried to find where we do this in our code and I didn't find it. I seems that we only look for mkdocs.yml at the root of the project:

https://github.com/rtfd/readthedocs.org/blob/ea7d37ee85b7659497bf130c72eaa085107f2cad/readthedocs/doc_builder/backends/mkdocs.py#L62-L72

Reading that code, it seems that we look into the project's root directory and if we don't find it we return None. Then, we create a mkdocs.yml with just one entry site_name and add the docs_dir (one of docs, doc, Doc, book if any of them exists in that order) at

https://github.com/rtfd/readthedocs.org/blob/ea7d37ee85b7659497bf130c72eaa085107f2cad/readthedocs/doc_builder/backends/mkdocs.py#L106

So, looking at the code it seems that we require a mkdocs.yml in the root.

I'm a little confused, tough.

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 13, 2018

The recomended layout for a mkdocs project is

mkdocs.yml
docs/
    index.md

https://www.mkdocs.org/user-guide/writing-your-docs/#file-layout

So having

README
docs/
    mkdocs.yml
    docs/
         index.md

would be a little weird...

Maybe having

README
myproject/
    mkdocs.yml
    docs/
         index.md

makes more sense. So +1 to edit the docs to reflect that.

Then, we create a mkdocs.yml with just one entry site_name and add the docs_dir (one of docs, doc, Doc, book if any of them exists in that order)

What @humitos mention is maybe what the docs wanted to say.

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 13, 2018

Well, looks like the docs already mention that we only search in the root of the project

@AumitLeon
Copy link
Contributor Author

@AumitLeon AumitLeon commented Aug 13, 2018

That makes sense to me, and I just confirmed that not having mkdocs.yml in the root of the repo causes the build to generate a new mkdocs.yml: AumitLeon/module_starter_cli#35

I think the docs don't mention that .md files should live in either a docs, doc, Doc, or book directory, so I'll add that.

@AumitLeon
Copy link
Contributor Author

@AumitLeon AumitLeon commented Aug 13, 2018

I'm not sure why the Travis build failed, I only modified the wording within build.rst to include info on where .md files should live. Any thoughts on what might have caused that?

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 13, 2018

Travis failed because of time out, I just restarted the jobs

docs/builds.rst Outdated
@@ -46,7 +46,7 @@ we will first look for a ``mkdocs.yml`` file in the root of your repository.
If we don't find one,
we will generate one for you.

Then MkDocs will build any files with a ``.md`` extension.
Then MkDocs will build any files with a ``.md`` extension within a single directory called ``docs``, ``doc``, ``Doc``, or ``book`` (this is the order in which directories are searched).
Copy link
Member

@stsewd stsewd Aug 13, 2018

I think this should be a little more clear since rtd only search in this directories if it doesn't find a mkdocs.yml file

Copy link
Contributor Author

@AumitLeon AumitLeon Aug 13, 2018

Got it. I can add a bit more detail to make the entire process more clear.

stsewd
stsewd approved these changes Aug 13, 2018
Copy link
Member

@stsewd stsewd left a comment

Thanks!

Copy link
Member

@humitos humitos left a comment

I left a question.

I think it's the only needed to merge this PR.

docs/builds.rst Outdated
Then MkDocs will build any files with a ``.md`` extension within the directory specifed as ``docs_dir`` in the ``mkdocs.yml``.

If no ``mkdocs.yml`` was found in the root of your repository and we generated one for you,
MkDocs will attempt to set ``docs_dir`` by sequentially searching for a ``docs``, ``doc``, ``Doc``, or ``book`` directory.
Copy link
Member

@humitos humitos Aug 15, 2018

I think here it should say Read the Docs will attempt instead of MkDocs will attempt since this is something that RTD code does, right?

Copy link
Contributor Author

@AumitLeon AumitLeon Aug 15, 2018

I agree-- that sounds better. I'll add that change!

@AumitLeon
Copy link
Contributor Author

@AumitLeon AumitLeon commented Aug 19, 2018

Thanks for all your comments! Is there anything else I should change before this PR gets merged in?

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 20, 2018

From my side, I don't think so. We only need to someone from the core team the press the merge button :). Sorry if this takes a while

@AumitLeon
Copy link
Contributor Author

@AumitLeon AumitLeon commented Aug 20, 2018

No worries at all, just happy to contribute!

Copy link
Member

@ericholscher ericholscher left a comment

Looks great, thanks!

@ericholscher ericholscher merged commit 6c0349a into readthedocs:master Aug 21, 2018
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants