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

md5 hash changes multiple times #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ dist/
django_static_compress.egg-info/
__pycache__
.vscode
.idea
3 changes: 3 additions & 0 deletions integration_test/statictest/static/base_styles.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
body {
background: #d3d6d8 url(body_bg.png); /* relative */
}
Binary file added integration_test/statictest/static/body_bg.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions integration_test/statictest/static/has_imports..css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import url("/static/base_styles.css"); /* absolute */
2 changes: 1 addition & 1 deletion integration_test/statictest/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

class CollectStaticTest(SimpleTestCase):
def setUp(self):
self.temp_dir = tempfile.TemporaryDirectory()
self.temp_dir = tempfile.TemporaryDirectory(prefix='dsc_')
whs marked this conversation as resolved.
Show resolved Hide resolved
self.temp_dir_path = Path(self.temp_dir.name)

def tearDown(self):
Expand Down
54 changes: 36 additions & 18 deletions static_compress/mixin.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
from collections import OrderedDict
from os.path import getatime, getctime, getmtime
import errno

Expand Down Expand Up @@ -34,7 +35,7 @@ def __init__(self, *args, **kwargs):
self.allowed_extensions = getattr(settings, "STATIC_COMPRESS_FILE_EXTS", ["js", "css", "svg"])
self.compress_methods = getattr(settings, "STATIC_COMPRESS_METHODS", DEFAULT_METHODS)
self.keep_original = getattr(settings, "STATIC_COMPRESS_KEEP_ORIGINAL", True)
self.minimum_kb = getattr(settings, "STATIC_COMPRESS_MIN_SIZE_KB", 30)
self.minimum_kb = getattr(settings, "STATIC_COMPRESS_MIN_SIZE_KB", 0)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not ok with changing this. Compressing everything make building slower and it is not always smaller.

If you want deterministic file hash from your web server, either fix the web server or change the library's configuration value.

Copy link
Author

@gassan gassan Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question here is. should I make it once or let my web server do it for me multiple times.
look at output of:
# strace -p $NGINX_WORKER_PID 2>&1 | grep .gz
On this art of errors generates nginx .gz file on fly if gzip = on; is set.
if not, so error 404 will be sent.

Edit: precondition for e404 here gzip_static = on;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can either turns gzip off in your web server, or set STATIC_COMPRESS_MIN_SIZE_KB=0 in your site's configuration.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, its right

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but with >0 not to exclude that it comes to the 404 error with default settings. so consider the user your module faulty and uninstall it:)

Copy link
Owner

@whs whs Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you hardcoding .gz/.br in your page?

The installation guide recommends that your web server should be configured to serve precompressed files.

Copy link
Author

@gassan gassan Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. I am using CompressedManifestStaticFilesStorage und yes gzip_static is on.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If gzip is off, i get 404 because requested file is C.hashC.css but exists as gzipped only A.OriginalHash.css
A, B are intermediate und C final versions of a certain file.


valid = [i for i in self.compress_methods if i in METHOD_MAPPING]
if not valid:
Expand Down Expand Up @@ -71,47 +72,64 @@ def get_modified_time(self, name):
return self._datetime_from_timestamp(getmtime(alt))

def post_process(self, paths, dry_run=False, **options):
if hasattr(super(), "post_process"):
yield from super().post_process(paths, dry_run, **options)

if dry_run:
return

files = OrderedDict()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear why this file has to be changed, based on the pull request's description.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look at super() class. with --dry-run no files are generated


super_class = super()
if hasattr(super_class, "post_process"):
generator = super_class.post_process(paths, dry_run, **options)
else:
generator = iter((f, f, False) for f in paths)

for file, hashed_file, processed in generator:
files[file] = hashed_file
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not exactly clear what this is doing. Consider adding commenting.

I'm guessing that it is to not process files that are not emitted by super class. If possible please consider splitting this to another PR so we can review this easier.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure here.
a super class must not have a post_process() method implemented. so you have to take in this case the whole fileset, that have not been post_processed

yield file, hashed_file, processed

for name in paths.keys():
if not self._is_file_allowed(name):
if not (name in files and self._is_file_allowed(name)):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name not in files or not self._is_file_allowed(name)

Copy link
Author

@gassan gassan Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a matter of taste and legibility :)

continue

source_storage, path = paths[name]
source_storage, _ = paths[name]

path = files[name]
# Process if file is big enough
if os.path.getsize(self.path(path)) < self.minimum_kb * 1024:
real_path = self.path(path)
if self.minimum_kb and os.path.getsize(real_path) < self.minimum_kb * 1024:
whs marked this conversation as resolved.
Show resolved Hide resolved
continue
src_mtime = source_storage.get_modified_time(path)
dest_path = self._get_dest_path(path)
with self._open(dest_path) as file:

with self._open(real_path) as file:
for compressor in self.compressors:
dest_compressor_path = "{}.{}".format(dest_path, compressor.extension)
dest_compressed_name = "{}.{}".format(path, compressor.extension)
dest_compressed_path = self.path(dest_compressed_name)

# Check if the original file has been changed.
# If not, no need to compress again.
full_compressed_path = self.path(dest_compressor_path)
try:
dest_mtime = self._datetime_from_timestamp(getmtime(full_compressed_path))
file_is_unmodified = dest_mtime.replace(microsecond=0) >= src_mtime.replace(microsecond=0)
if hasattr(self, 'hashed_name'): # compute on hash value. if file exists so is it up to date
file_is_unmodified = self.exists(dest_compressed_name)
else:
src_mtime = source_storage.get_modified_time(path)
dest_mtime = self._datetime_from_timestamp(getmtime(dest_compressed_path))
file_is_unmodified = dest_mtime.replace(microsecond=0) >= src_mtime.replace(microsecond=0)
except FileNotFoundError:
file_is_unmodified = False
if file_is_unmodified:
yield path, dest_compressed_name, False
continue

# Delete old gzip file, or Nginx will pick the old file to serve.
# Note: Django won't overwrite the file, so we have to delete it ourselves.
if self.exists(dest_compressor_path):
self.delete(dest_compressor_path)
if self.exists(dest_compressed_path):
self.delete(dest_compressed_path)
out = compressor.compress(path, file)

if out:
self._save(dest_compressor_path, out)
self._save(dest_compressed_path, out)
if not self.keep_original:
self.delete(name)
yield dest_path, dest_compressor_path, True
yield path, dest_compressed_name, True

file.seek(0)

Expand Down