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

@davidfischer davidfischer commented Aug 7, 2018

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 davidfischer requested a review from agjohnson Aug 7, 2018
@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Aug 7, 2018

This line might hide some problems as well.

Loading

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

@davidfischer davidfischer Aug 8, 2018

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.

Loading

Copy link
Member

@humitos humitos left a comment

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

Loading

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

@humitos humitos Aug 8, 2018

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?

Loading

Copy link
Contributor Author

@davidfischer davidfischer Aug 8, 2018

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.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Aug 8, 2018

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.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Aug 8, 2018

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

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Aug 9, 2018

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.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Aug 9, 2018

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.

Loading

agjohnson added a commit that referenced this issue 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

@davidfischer davidfischer commented Aug 10, 2018

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

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Aug 14, 2018

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

Loading

@ericholscher ericholscher merged commit ce3e4f9 into master Aug 14, 2018
1 check passed
Loading
@stsewd stsewd deleted the davidfischer/all-static-collectstatic branch Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants