nodemcu / nodemcu-firmware

Lua based interactive firmware for ESP8266, ESP8285 and ESP32
https://nodemcu.readthedocs.io
MIT License
7.61k stars 3.12k forks source link

MkDocs local build links broken #2653

Closed galjonsfigur closed 5 years ago

galjonsfigur commented 5 years ago

Expected behavior

Local documentation build works fine

Actual behavior

In every module documentation page, menu links to other sections are broken.

After testing various ways to build documentation in local environment without warnings I found that in index page every link was correct, but in any module page menu links to other sections became broken (linking to GitHub instead of internal pages). After checking various versions of MkDocs and python 2/3 I found that there is a bug in /docs/js/extra.js file that replaces links incorrectly because of commit https://github.com/nodemcu/nodemcu-firmware/commit/6a485568a52a7ac13527fb19b909253890d2d7f7 - I wasn't aware of this file until now. If no one would want to fix JS code, I can try to do this - the problem is in this function.

marcelstoer commented 5 years ago

Thanks for reporting

because of commit 6a48556

I didn't analyze in detail but I suspect the bug has been there for much longer as that commit simply moved all the content one directory up.

galjonsfigur commented 5 years ago

I didn't analyze in detail but I suspect the bug has been there for much longer as that commit simply moved all the content one directory up.

And that seems to be a problem - in generated HTML files links look like this (in any lua module documentation) <a class="" href="../../sdcard/">Filesystem on SD card</a> or for any C module <a class="" href="../../modules/bme680/">bme680</a>

And the fact that docs were moved from /docs/en/modules to docs/modules doesn't matter - relative links were the same for both I think.

marcelstoer commented 5 years ago

The relative links went from to ../../../ to ../../ and the JS function followed suit.

galjonsfigur commented 5 years ago

I'm trying to fix it and it seems that it's because of the way MkDocs generates links to other pages - the JS script misinterprets those paths. The only simple solution i can see is to move all .md files to new md folder (or any other name - so it would be the old way) or change the script to only manipulate links in documentation and not the menu, but I don't think it's the best idea.

marcelstoer commented 5 years ago

At this point I'm not sure which problem you/we are actually trying to solve. As we discussed in #2645 you can't have relative links to artifacts outside the docs folder. Symlinks might be a solution to this. Most relative links within a module API documentation page e.g. http://127.0.0.1:8000/modules/file/ work just fine. Same on https://nodemcu.readthedocs.io/en/latest/modules/file/ and https://github.com/nodemcu/nodemcu-firmware/blob/dev/docs/modules/file.md.

What is definitely broken is that, when run locally, most links in the navigation tree point to GitHub instead of to the local copy.

galjonsfigur commented 5 years ago

The most important problem I'm trying to solve now is that local build navigation is broken. For example: In http://127.0.0.1:8000/lua-modules/ds3231/ every navigation link outside Lua Modules section points to nonexistent github page. Because MkDocs uses relative links and commit https://github.com/nodemcu/nodemcu-firmware/commit/6a485568a52a7ac13527fb19b909253890d2d7f7 moved everything one directory up, there is no way to distinguish links to source files from links to documentation pages from other sections.

The problem with local links to source files is not that urgent - I'm still looking for better solution than symlinks because some IDE indexers are confused by them and on Windows environment they require Administrator privileges AFAIK (I'm not using Windows BTW but I'm sure that someone can and that would be a problem for that person).

marcelstoer commented 5 years ago

I'm tempted to commit something like this for extra.js but further testing is necessary.

diff --git a/docs/js/extra.js b/docs/js/extra.js
index e2fef36..92f7514 100644
--- a/docs/js/extra.js
+++ b/docs/js/extra.js
@@ -44,12 +44,14 @@ var nodemcu = nodemcu || {};
    * replaces the relative path with an absolute path based on the selected branch.
    */
   function replaceRelativeLinksWithStaticGitHubUrl() {
-    var relativePath = isOnRtd() ? "../../.." : "../..";
-    var gitHubPath = "https://github.com/nodemcu/nodemcu-firmware/tree/" + determineSelectedBranch();
-    var gitHubLinks = $("a[href^='" + relativePath + "']").each(function (index) {
-      var url = $(this).attr('href');
-      $(this).attr('href', url.replace(relativePath, gitHubPath));
-    });
+    if (isOnRtd()) {
+      var relativePath = "../../..";
+      var gitHubPath = "https://github.com/nodemcu/nodemcu-firmware/tree/" + determineSelectedBranch();
+      var gitHubLinks = $("a[href^='" + relativePath + "']").each(function (index) {
+        var url = $(this).attr('href');
+        $(this).attr('href', url.replace(relativePath, gitHubPath));
+      });
+    }
   }

   /**

-> for testing locally and for GitHub the relative links remain as-is

marcelstoer commented 5 years ago

@galjonsfigur could you please verify that the proposed fix works ok for you, too?

galjonsfigur commented 5 years ago

I tested this on both local (mkdocs 1.x.x) and RTD instance (also 1.x.x) - It works, but without symlinks to docs folder links to source files won't work. But it's much better - I think it's good to assume that if someone uses local documentation it won't be a problem for that person to find a source file. I also tested symlink approach - works fine for files but not folders. Again - I don't really see the problem with that because documentation shares this repository with all the code. An 'sh and/or .bat script could be added as an one-time script to set up links.

👍 from my side.

Also now it could be a good idea to bump version of mkdocs since that works OK and its easier to just install latest mkdocs instead of looking how to install specific version - the only thing that would be necessary is to change rtd-requirements.txt and change pages to nav in mkdocs.yml but that's thing for other PR.

marcelstoer commented 5 years ago

I tested this on both local (mkdocs 1.x.x) and RTD instance (also 1.x.x) - It works

Thanks for testing. I'll commit it now (-> f0a240aa46c3b844487a5e184b5cfa88eec11365).

An 'sh and/or .bat script could be added as an one-time script to set up links.

Yes, that's what I had in mind. However, the script might as well do a mkdocs serve at the end and the Unix version could be added as new make target to support something like make docs.

Also now it could be a good idea to bump version of mkdocs since that works OK and its easier to just install latest mkdocs instead of looking how to install specific version - the only thing that would be necessary is to change rtd-requirements.txt and change pages to nav in mkdocs.yml but that's thing for other PR.

👍 I haven't tried to build on RTD with MkDocs 1.x in a while because they kept struggling to support that properly. Great to learn that this is now fixed.