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

Broken links/css in readthedocs 404 page #1317

Closed
brianhlin opened this issue Oct 20, 2017 · 11 comments
Closed

Broken links/css in readthedocs 404 page #1317

brianhlin opened this issue Oct 20, 2017 · 11 comments
Labels

Comments

@brianhlin
Copy link

@brianhlin brianhlin commented Oct 20, 2017

The 404.html added in 0.17.0 seems to have broken links and css (failing CI build). The links in the generated 404.html file all start with a docs/... prefix but when I inspect the site dir after a mkdocs build, there is no docs directory.

@waylan
Copy link
Member

@waylan waylan commented Oct 20, 2017

Hmm, its working fine for me (see mkdocs.org). I checked the readthedocs theme locally, and it works fine also. So I looked at your project.

Then I noticed that you have site_url: https://opensciencegrid.github.io/docs set in your config file. As the 404 page could be shown for any URL, it can't use relative URLS. Therefore, it uses site_url to derive an absolute URL. In your case, it takes /docs from the site_url setting and prepends it to the URL of each link on the page. Of course, when running the dev server locally, that shouldn't apply.

At the time that we implemented the absolute URLs that way, the dev server didn't have support for returning a 404 page (just a simple plain-text message), so we weren't concerned that it didn't work there as it does on the production server. Since then, we got 404 support added to the dev server and presumably forgot about that detail.

Thanks for the report. We'll need to fix this. However, be aware that while your patch (via a custom 404 template) appears to resolve the problem in the dev server, it will have broken things on the production server if/when you deploy it. I would recommend removing it and enduring the broken dev server until we fix this in 0.17.1 (I hope).

Loading

@waylan waylan added the Bug label Oct 20, 2017
@brianhlin
Copy link
Author

@brianhlin brianhlin commented Oct 20, 2017

Thanks for the quick reply! The custom 404 seems to be working in production (https://opensciencegrid.github.io/docs/site-planning/s) so we'll probably just keep things this way until this is fixed.

Loading

@waylan
Copy link
Member

@waylan waylan commented Oct 20, 2017

@brianhlin was the default 404 page broken for you in production or only on the dev server? I assume only on the dev server, but if not, then we have a more serious problem.

For future reference (for myself mostly) , a detailed explanation of the relative/absolute problem is in this comment. #1039 provided the fix for error pages with an acknowledgement that it doesn't work on the dev server. And we added error page support to the dev server in #1141 (where we forgot to address this issue).

Loading

@brianhlin
Copy link
Author

@brianhlin brianhlin commented Oct 20, 2017

I hadn't bothered to push the change to production, previously. I tested it on my own fork and it looks like you're right, it works in production.

Loading

waylan added a commit to waylan/mkdocs that referenced this issue Oct 20, 2017
When serving locally, the `site_url` actually is the `dev_addr`, so the
config should relfect that. Note that this override must happen after
config validation so that the proper defaults are evaluated for the
`dev_addr` setting. It cannot happen as part of "post_validation" as
there is no way to know the dev server is running within the config
validation (for example, the dev_addr may not be None as the user could
have set the `dev_addr` setting in their config file). Overriding it
from the serve command is the least invasive method and guarantees it
only happens when the local dev server is being run.

Also cleaned up `dev_addr` docs, which were missleading in some respects
and actually encouraged using the dev server over a network.

Fixes mkdocs#1317. 404 Error pages now work on the loval dev server when
the `site_url` in the config points to a subdir.
waylan added a commit to waylan/mkdocs that referenced this issue Oct 20, 2017
When serving locally, the `site_url` actually is the `dev_addr`, so the
config should relfect that. Note that this override must happen after
config validation so that the proper defaults are evaluated for the
`dev_addr` setting. It cannot happen as part of "post_validation" as
there is no way to know the dev server is running within the config
validation (for example, the dev_addr may not be None as the user could
have set the `dev_addr` setting in their config file). Overriding it
from the serve command is the least invasive method and guarantees it
only happens when the local dev server is being run.

Also cleaned up `dev_addr` docs, which were missleading in some respects
and actually encouraged using the dev server over a network.

Fixes mkdocs#1317. 404 Error pages now work on the loval dev server when
the `site_url` in the config points to a subdir.
@waylan
Copy link
Member

@waylan waylan commented Oct 20, 2017

I just pushed a fix for this in #1318. As this effects the dev server only, we'll wait to see if any more bugs surface before doing a release. Either way, this will be fixed in the 0.17.1 release when it comes out. Thanks again for the report.

Loading

@waylan waylan closed this in #1318 Oct 20, 2017
waylan added a commit that referenced this issue Oct 20, 2017
When serving locally, the `site_url` actually is the `dev_addr`, so the
config should relfect that. Note that this override must happen after
config validation so that the proper defaults are evaluated for the
`dev_addr` setting. It cannot happen as part of "post_validation" as
there is no way to know the dev server is running within the config
validation (for example, the dev_addr may not be None as the user could
have set the `dev_addr` setting in their config file). Overriding it
from the serve command is the least invasive method and guarantees it
only happens when the local dev server is being run.

Also cleaned up `dev_addr` docs, which were missleading in some respects
and actually encouraged using the dev server over a network.

Fixes #1317. 404 Error pages now work on the loval dev server when
the `site_url` in the config points to a subdir.
@brianhlin
Copy link
Author

@brianhlin brianhlin commented Nov 29, 2017

This still appears to be broken for 1.17.1 (failing CI):

URL        `/management/img/favicon.ico'
Parent URL file:///home/travis/build/opensciencegrid/management/site/404.html, line 10, col 3
Real URL   file:///management/img/favicon.ico
Check time 0.000 seconds
Result     Error: URLError: <urlopen error [Errno 2] No such file or directory: '/management/img/favicon.ico'>

Loading

brianhlin added a commit to brianhlin/doc-ci-scripts that referenced this issue Nov 29, 2017
brianhlin added a commit to opensciencegrid/doc-ci-scripts that referenced this issue Nov 29, 2017
brianhlin added a commit to opensciencegrid/doc-ci-scripts that referenced this issue Nov 29, 2017
@waylan
Copy link
Member

@waylan waylan commented Nov 29, 2017

So the fix for this issue was specifically related to the dev server. However, your errors are all related to file:// URLs, which is a completely different animal. For example, consider the quoted error you provide above. The URL is /management/img/favicon.ico. From a server, that would work correctly as /management is at the server root, However, for a file:// based URL, that would only work if the management dir is at the root of your file system. Of course its not. I'm assuming from your CI logs that its actually at /home/travis/build/opensciencegrid/management.

Using the file:// scheme means that each build on each system would be completely different. Even builds at different locations on the same system would be different. Therefore, we don't officially support building a site to be used with the file:// scheme. Sure, for the most part you can browse the site (because we use relative URLs everywhere we can), but search and error pages don't work (because we need absolute URLs there). Of course without a server to intercept a request for a missing page, error pages are pointless, so we will never support error pages with the file:// scheme. We do intend to get search working with the file:// scheme in an upcoming refactor of search, but that is a separate issue.

So I was curious why we don't hit the same error for MkDoc's linkchecker tests and I don't see any linkchecker config in your repo. However, the Travis log indicates you are calling linkchecker like this: linkchecker --config=ci/linkchecker.config site/index.html. But we already established that your server root is /management, so that needs to be passed to linkchecker. In fact, MkDocs passes the absolute path to the server root to linkchecker (linkchecker {envtmpdir}/builds/). Of course, we have the envtmpdir variable which get proper expanded because we're using tox, but you would need to effectively do the same thing. In other words, this is a configuration error on your part. You need to go through some gymnastics to make linkchecker work without your site behind a server. Or possibly, you could have it skip the error page.

Loading

@brianhlin
Copy link
Author

@brianhlin brianhlin commented Nov 29, 2017

Sorry, I don't see how it's a configuration. It still strikes me as brokenness in 404.html. Note that I'm running mkdocs build, which by default is building into site/, relative to my local repository:

$ PYTHONPATH=src mkdocs build
INFO    -  Cleaning site directory 
INFO    -  Building documentation to directory: /home/blin/docs/management/site 

Do you really need absolute URLs for search and error pages? Because the root index.html seems to be using relative URLs:

$ pwd
/home/blin/docs/management
$ grep search.js site/index.html 
      <script src="./search/search.js"></script>

Doing the same check on 404.html, however, shows an incorrect absolute path.

$ grep search.js site/404.html 
      <script src="/management/search/search.js"></script>

Loading

@waylan
Copy link
Member

@waylan waylan commented Nov 29, 2017

Do you really need absolute URLs for search and error pages?

Yes, absolute URLs are really necessary for error pages, as explained here (which was also linked above). True, absolute URLs are not necessary for search, but a fix for that is in the works (unfortunately, the way search is currently implemented that fix needs to be a complete refactor of the relevant JS).

Note that I'm running mkdocs build, which by default is building into site/, relative to my local repository.

Yes, but you have site_url: https://opensciencegrid.github.io/management/ in your config, so the absolute path to the home page is /management/, not /. The fix we applied causes the site_url setting to be ignored when running the dev sever (so the absolute path to the home page is /). But you're not running the dev server so that setting is not getting ignored. Normally that would be fine. The one exception is if you are using the file:// scheme. And we don't care if error pages don't work with the file:// scheme because they are pointless in that context.

In other words, the mkdocs config (the absolute path to the home page being /management/) does not match your testing environment. Either you need to make your test environment match your config, or your config match your test environment.

So you have a few options:

  1. Run a server and have linkchecker run against that server.
  2. Build without /management/ in your site_url.
  3. Adjust your test envionment to account for the absolute URLs in the error page having a root of /management (and search until the refactor is completed).
  4. Configure linkchecker to ignore the error page (and search...).

Ideally, I would prefer option 1 (as the test environment would match the production environment), but you then need to get Travis to shut the server down after the test completes, and I'm not sure how to get that to work. Option 2 is not possible via the build command (there's no --site_dir flag), so you would need to modify mkdocs.yml (perhaps via a script?), which is less than ideal. That leaves options 3 & 4, which I previously suggested,

Loading

@waylan
Copy link
Member

@waylan waylan commented Nov 29, 2017

Oh one more note: The reason we don't encounter this problem in MkDocs' tests is that we have site_url: http://mkdocs.org/ so our server root is /. This problem only exists when the server root in site_url is something other than / and you use the file:// scheme. But that is a different (albeit related) issue than reported here. The reported issue here is about the dev server, which was fixed.

Loading

@brianhlin
Copy link
Author

@brianhlin brianhlin commented Nov 29, 2017

The issue I originally reported was actually concerning mkdocs build. I was just under the impression that your fix for the dev server would address the issue in mkdocs build as well but I see now why it wouldn't. Thanks for the thorough explanation.

Loading

brianhlin added a commit to brianhlin/doc-ci-scripts that referenced this issue Nov 29, 2017
brianhlin added a commit to opensciencegrid/doc-ci-scripts that referenced this issue Dec 16, 2017
brianhlin added a commit to opensciencegrid/doc-ci-scripts that referenced this issue Dec 22, 2017
This is closer to production than 'mkdocs build' and verifying
file:/// links. The alternative solution to a mkdocs server is to
deploy with a root-enabled Travis-CI image and building into /<repo>
but decided against that since root images dramatically increases CI
build start time

mkdocs/mkdocs#1317 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants