Opened 9 months ago

Last modified 9 months ago

#27854 assigned Cleanup/optimization

Make `collectstatic` warn (rather than blow up) on missing directories

Reported by: David Evans Owned by: Mahesh Kumar Jurel
Component: contrib.staticfiles Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (13)

comment:1 Changed 9 months ago 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?

comment:2 in reply to:  1 Changed 9 months ago by David Evans

Replying to 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:

  1. 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.
  2. 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".

comment:3 Changed 9 months ago 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" :-)

comment:4 in reply to:  3 Changed 9 months ago by David Evans

Replying to 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.

comment:5 Changed 9 months ago 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.

comment:6 Changed 9 months ago by Tim Graham

Another trend is moving runtime warnings to the system check framework, e.g. 0ec4dc91e0e7befdd06aa0613b5d0fbe3c785ee7. Could that be appropriate here?

comment:7 in reply to:  6 Changed 9 months ago 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?

comment:8 Changed 9 months ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:9 Changed 9 months ago by Mahesh Kumar Jurel

Owner: changed from nobody to Mahesh Kumar Jurel
Status: newassigned

comment:10 Changed 9 months ago by Mahesh Kumar Jurel

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

comment:11 Changed 9 months ago by Mahesh Kumar Jurel

Resolution: fixed
Status: assignedclosed

comment:12 Changed 9 months ago by Tim Graham

Has patch: set
Needs tests: set
Resolution: fixed
Status: closednew

The ticket is marked fixed when the patch is committed to the Django repository. Please read Triaging Tickets. A test is also required. Please uncheck "Needs tests" on the ticket after updating the PR.

comment:13 Changed 9 months ago by Mahesh Kumar Jurel

Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top