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

All static media is run through "collectstatic" #4489

Merged
merged 4 commits into from
Aug 14, 2018

Conversation

davidfischer
Copy link
Contributor

This changes how our static files are collected. Rather than media/ being a directory where static files are served from and collectstatic writes to media/static, this turns the media/ directory into just another entry in STATICFILES_DIRS and all static media is collected into a new directory static/.

This is a somewhat difficult PR to test thoroughly and it is possible that issues we don't anticipate might arise from it. It makes me a bit nervous to be honest. One challenge is that a lot of built documentation still uses /media/ (eg. /media/javascript/jquery/jquery-migrate-1.2.1.min.js)

@davidfischer
Copy link
Contributor Author

This line might hide some problems as well.

@@ -1,12 +0,0 @@
<!DOCTYPE HTML>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file does not appear to be used anywhere. I took a look at the commit that added it 6 years ago (38ca662) and most of that code is gone.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still trying to understand how our static/media works. So, maybe my questions are irrelevant.

One challenge is that a lot of built documentation still uses /media/

How these docs will behave? Will they fail to load the resources?

Also, I want to raise a question here to anticipate a possible issue: how this will affect the corporate site? It looks that we already have a question there:

https://github.com/rtfd/readthedocs-corporate/blob/0f7590f4674028a539673b331194cd2d0ed7cdc6/readthedocsinc/settings/base.py#L68-L77

../../readthedocs/static/vendor/underscore-standalone.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that you just updated the path of the link, but why we are adding static files from our app into the media directory? Shouldn't this be served from the STATICFILES_DIRS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's two problems:

  • Firstly, collectstatic actually checks that every symlink resolves. Since /media/static won't exist if this PR is merged, any symlink referring to there needs updating.
  • The second problem is that some already built docs refer to paths that are referenced by symlinks. For example, <script type="text/javascript" src="https://media.readthedocs.org/javascript/underscore.js"></script> which is in all the built docs relies on this symlink.

@davidfischer
Copy link
Contributor Author

How these docs will behave? Will they fail to load the resources?

As long as we continue to serve the media directory, no. Longer term, we should probably redirect or something else.

@davidfischer davidfischer added the PR: work in progress Pull request is not ready for full review label Aug 8, 2018
@davidfischer
Copy link
Contributor Author

I went ahead and tagged this "work in progress". I don't think this PR will get merged as-is.

@davidfischer
Copy link
Contributor Author

As long as we continue to serve the media directory, no. Longer term, we should probably redirect or something else.

I updated this PR to redirect for those resources (eg. /media/javascript/jquery/jquery-migrate-1.2.1.min.js -> /static/javascript/jquery/jquery-migrate-1.2.1.min.js). In production, this should be a redirect at the web server level, not Django. Once this is rolled out, we should update various bits of code (in https://github.com/rtfd/readthedocs-sphinx-ext for example) to use the new URL.

@davidfischer
Copy link
Contributor Author

Another option is to move all the files under media/ to readthedocs/static and leave media/ exclusively for build artifacts. Then media/ could be removed from STATICFILES_DIRS. We would still need to redirect /media -> /static for non-build artifact files though.

agjohnson added a commit that referenced this pull request Aug 9, 2018
This doesn't change the STATICFILES_ROOT yet, in an attempt to avoid making any
breaking changes. This is a port of #4489 that we'll merge before azure
migration
@davidfischer
Copy link
Contributor Author

I think the plan with this PR is to merge #4502 first (before the Azure move) and then merge this relatively soon after.

@ericholscher
Copy link
Member

I've merged master in after #4502, and now will merge this in.

@ericholscher ericholscher merged commit ce3e4f9 into master Aug 14, 2018
@stsewd stsewd deleted the davidfischer/all-static-collectstatic branch August 15, 2018 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants