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

Fixes bug in compiler modified_time handling #212

Merged
merged 1 commit into from
May 2, 2013
Merged

Fixes bug in compiler modified_time handling #212

merged 1 commit into from
May 2, 2013

Conversation

davehughes
Copy link
Contributor

I had the same issue as #211 (and possibly #210) and this seems to be a reasonable solution. When I dug into the issue, I found:

  • The compiler passes absolute file paths for infile and outfile when trying to determine modification times.
  • The modified_time method on Django's file storage objects uses the storage's path() method as part of its determination
  • path() expects a storage-relative name, which is at odds with passing absolute file paths
  • path() doesn't find the named file, and falls back to the default StaticFilesStorage handling, which for some reason looks for the absolute file in the static directory of the first app in my INSTALLED_APPS, notices that the requested file is outside of that directory, and raises a SuspiciousOperation error.

FYI, I'm relying on the `django.contrib.staticfiles.finders.AppDirectoriesFinder' finder, which may be part of the cause of this issue.

@davehughes
Copy link
Contributor Author

I also tried writing a test or two to reproduce this behavior, but I'm fairly new to tox and wasn't able to come up with a good way to exercise the compiler side of things (LESS, in my case). Do you have any pointers for installing Node/NPM and using them to test compilation with pipeline?

@cyberdelia
Copy link
Member

Thanks, I'm afraid we might be missing some corner case here, dealing with app directories is hard.

No good pointer, unfortunately, except that it's a mess 😑
And testing for one compiler is pointless, they all work differently 😢

@cyberdelia
Copy link
Member

Oh wait! You're totally right. I'll merge this and make a releaseI
If you still want to make some test case, please do, but don't try to run less or something similar, you can create your own dummy compiler that mock what you expect. And don't let tox intoxicate you.
Thank you, you rock! ✨

cyberdelia added a commit that referenced this pull request May 2, 2013
Fixes bug in compiler modified_time handling
@cyberdelia cyberdelia merged commit 2555f99 into jazzband:master May 2, 2013
StoicLoofah referenced this pull request Mar 22, 2016
The check to see whether or not a source file is outdated was being passed the
`input_path` and `output_path` variables, which are not the actual paths to
files on disk. In particular, `input_path` is the path which will be searched
for using the staticfiles finders, and `output_path` is the relative path
within the staticfiles root. If the staticfiles root was not the same as the
root directory of the project, this would result in the check always reporting
that the file was outdated. In addition, if a source file required a finder to
locate, the check would fail.

Changing this to use `infile` and `outfile` instead means that the check
operates on the same file paths that the compiler will. This therefore checks
the files that were copied by the collector against the files which are
outputted by the compiler, which is a much more correct idea of whether
something is out of date.

This was tested in conjunction with a custom LessCompiler that uses a helper to
introspect dependencies. Before this change, that helper was seeing file paths
that did not exist (since STATIC_ROOT is a few subdirectories deep in our
codebase). Afterwards, it is able to successfully introspect all the source
files to build the dependency tree.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants