Opened 7 years ago

Closed 3 years ago

#27854 closed Cleanup/optimization (fixed)

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

Reported by: David Evans Owned by: Jacob Walls
Component: contrib.staticfiles Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no 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 (20)

comment:1 by Tim Graham, 7 years ago

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?

in reply to:  1 comment:2 by David Evans, 7 years ago

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 by Aymeric Augustin, 7 years ago

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" :-)

in reply to:  3 comment:4 by David Evans, 7 years ago

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 by Aymeric Augustin, 7 years ago

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 by Tim Graham, 7 years ago

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

in reply to:  6 comment:7 by David Evans, 7 years ago

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 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:9 by Mahesh Kumar Jurel, 7 years ago

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

comment:10 by Mahesh Kumar Jurel, 7 years ago

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 by Mahesh Kumar Jurel, 7 years ago

Resolution: fixed
Status: assignedclosed

comment:12 by Tim Graham, 7 years ago

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 by Mahesh Kumar Jurel, 7 years ago

Status: newassigned

comment:14 by Jacob Walls, 3 years ago

Owner: changed from Mahesh Kumar Jurel to Jacob Walls

comment:15 by Jacob Walls, 3 years ago

Needs tests: unset
Version: 1.10master

comment:16 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

comment:17 by Jacob Walls, 3 years ago

Patch needs improvement: unset

comment:18 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In be8faa7:

Refs #27854 -- Skipped subsequent checks if STATICFILES_DIRS is not a list or tuple.

comment:20 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In b23232b6:

Fixed #27854 -- Added system check for nonexistent directories in STATICFILES_DIRS setting.

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