[Django] #27854: Make `collectstatic` warn (rather than blow up) on missing directories
Django
-----------------------------------------------+------------------------
Reporter: David Evans | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------------+------------------------
At present if the `STATICFILES_DIRS` setting contains references to
directories which do not exist then the whole command will die with an
`OSError`.
A situation I've seen bite a few newcomers to Django is that they will
have an empty static directory which (being empty) won't get tracked by
git. This means that `collectstatic` works when they run it locally, but
blows up with a (to them) cryptic error when they try to deploy.
If we made `collectstatic` simply log a warning in the case of non-
existent directories and continue processing then we could avoid this
problem and remove one possible source of frustration for newcomers.
If this approach seems acceptable to others I am happy to submit an
appropriate patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/27854>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: David Evans | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Maybe the current error message could be improved. What does it say?
Did you research the origin of the current exception to see if you can
find any design decisions there?
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:1>
Django
Reporter: David Evans | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Evans):
Replying to [comment:1 Tim Graham]:
> Maybe the current error message could be improved. What does it say?
>
> Did you research the origin of the current exception to see if you can
find any design decisions there?
The exception is just `OSError: [Errno 2] No such file or directory`
This bubbles up from directly from the call to `os.listdir` here:
https://github.com/django/django/blob/1f7ca858664491589ba400419a491dd0a9af5dff/django/core/files/storage.py#L312
So I don't think there was any deliberate design decision here.
Also, I don't think improving the error message will solve this specific
problem because:
a. If the user is deploying to Heroku then they are just told that the
collectstatic step was skipped due to errors, but they have to run the
command themselves to see what those errors were.
b. An error message about missing directories is confusing to them because
they can see the directory right there on their filesystem. The fact that
git doesn't track empty directories is then yet another bit of weird
computer arcanery that we need to expose them to when they are just trying
to deploy "My First Website".
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:2>
Django
Reporter: David Evans | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin):
In the case of Heroku which swallows the output of collectstatic, if
`STATICFILES_DIRS` doesn't have a correct value, the change suggested here
will make it pretend that it succeeded, even though it didn't collect
files, while it currently says it's failing. I think that's a problem:
"errors shouldn't pass silently" :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:3>
Django
Reporter: David Evans | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Evans):
Replying to [comment:3 Aymeric Augustin]:
> In the case of Heroku which swallows the output of collectstatic, if
`STATICFILES_DIRS` doesn't have a correct value, the change suggested here
will make it pretend that it succeeded, even though it didn't collect
files, while it currently says it's failing. I think that's a problem:
"errors shouldn't pass silently" :-)
Yes, absolutely agree that errors shouldn't pass silently. In the case of
the Heroku buildpack, it does suppress some of the output but any warning
messages will get displayed:
https://github.com/heroku/heroku-buildpack-
python/blob/677dfeec119f28b4d1a8f679b38b2d4e407f4533/bin/steps/collectstatic#L33
Would another option be for `collectstatic` to note that a directory was
missing, issue a warning, ''proceed with collecting the rest of the static
files'', and then exit with status > 0 at the end? That way, the user
would still have a working set of static files.
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:4>
Django
Reporter: David Evans | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin):
As long as that doesn't obscure errors, yes, it should be an improvement.
Historically we've removed lots of "helpful" error handling which made
other errors harder to diagnose. Often the raw stack trace is the best way
to see what went wrong. I wanted to share that experience with the first
case that came to my mind.
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:5>
Django
Reporter: David Evans | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Another trend is moving runtime warnings to the system check framework,
e.g. 0ec4dc91e0e7befdd06aa0613b5d0fbe3c785ee7. Could that be appropriate
here?
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:6>
Django
Reporter: David Evans | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Evans):
That's an interesting idea. So referencing a non-existent directory in
`STATICFILES_DIRS` would trigger a warning in the system check framework,
but wouldn't prevent `collectstatic` from running. Aymeric, would that
address your concerns?
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:7>
Django
Reporter: David Evans | Owner: nobody
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:8>
Django
Reporter: David Evans | Owner:
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by maheshjurel):
* owner: nobody => maheshjurel
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:9>
Django
Reporter: David Evans | Owner:
Type: | maheshjurel
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by maheshjurel):
Changes have been made to show warning (without stopping execution) if any
of the static files directory does not exists. Earlier the situation was
that if we have more than one directory paths in `STATICFILES_DIRS` and
if the first one is missing then it wouldn't process the next directory
and fails giving the exception logs.
But now, with the current changes it will show a message in the format
that `"Path [<missing_dir_path>] does not exists. Skipping..."` and also
process all the directories (next ones) as expected.
A pull request has been created
[https://github.com/django/django/pull/8137]
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:10>
Django
Reporter: David Evans | Owner:
Type: | maheshjurel
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by maheshjurel):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:11>
Django
Reporter: David Evans | Owner:
Type: | maheshjurel
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: closed => new
* has_patch: 0 => 1
* resolution: fixed =>
* needs_tests: 0 => 1
Comment:
The ticket is marked fixed when the patch is committed to the Django
repository. Please read
[https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/ Triaging Tickets]. A test is also required. Please uncheck "Needs
tests" on the ticket after updating the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:12>
Django
Reporter: David Evans | Owner: Mahesh
Type: | Kumar Jurel
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mahesh Kumar Jurel):
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:13>
Django
Reporter: David Evans | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: Mahesh Kumar Jurel => Jacob Walls
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:14>
Django
Reporter: David Evans | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* version: 1.10 => master
* needs_tests: 1 => 0
Comment:
[https://github.com/django/django/pull/14056 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:15>
Django
Reporter: David Evans | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:16>
Django
Reporter: David Evans | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:17>
Django
Reporter: David Evans | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:18>
Django
Reporter: David Evans | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"be8faa7c7590d6b961ad9abdc65588f54194aaab" be8faa7]:
{{{
#!CommitTicketReference repository=""
revision="be8faa7c7590d6b961ad9abdc65588f54194aaab"
Refs #27854 -- Skipped subsequent checks if STATICFILES_DIRS is not a list
or tuple.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:19>
Django
Reporter: David Evans | Owner: Jacob
Type: | Walls
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b23232b6ab1b32969b018380922a6c550ba4f4aa" b23232b6]:
{{{
#!CommitTicketReference repository=""
revision="b23232b6ab1b32969b018380922a6c550ba4f4aa"
Fixed #27854 -- Added system check for nonexistent directories in
STATICFILES_DIRS setting.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27854#comment:20>