Change bundle path and fix relative URL's #41

cvan · 2014-11-12T23:33:10Z

forgive my ignorance. is this different from _clean_tmp?

it is this is getting rid of the temporary files that are being created for each one of the css files in which the URLs are corrected.

_clean_tmp just gets rid of the temporary file for each bundle.

cvan · 2014-11-12T23:33:49Z

good fix but seems unrelated

cvan · 2014-11-12T23:35:43Z

this should be optional. on Marketplace at least, we don't want this to happen.

I'll add a setting in to control this.

cvan · 2013-02-24T20:12:07Z

tests for this method would be great

i'll add some

mythmon · 2013-02-24T06:20:37Z

I've always been told this should be written as try: makedir; except: nvm, because race conditions.

cvan · 2013-02-24T20:10:32Z

why "'? I could be missing something, but shouldn't this be .strip('"').strip("'")?

Doing .strip('\'"') ditches both characters from left and right sides. You could do it the way you suggested, too:

$ python Python 2.7.3rc2 (default, Apr 22 2012, 22:35:38) [GCC 4.6.3] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> '"\'foo\'"'.strip('\'"') 'foo' >>> '"\'foo\'"'.strip('"').strip("'") 'foo' >>>

http://docs.python.org/2/library/stdtypes.html#str.strip

the more I learn, very nice

cvan · 2014-11-12T23:36:24Z

oh, nice catch

cvan · 2014-11-12T23:36:39Z

add a newline below to separate the import blocks

cvan · 2014-11-12T23:37:21Z

one line 88 you have the % on the line below. should be consistent.

davidbgk · 2014-04-09T14:31:33Z

I can't find where you use that package in the diff.

It's unrelated but missing. There's a test in #63 that demonstrates the issue. Any new tests that hit that area of code will trip over it.

cvan · 2014-11-12T23:38:32Z

why the added dependency? I don't see where it's used

-Original file line number
+Diff line change
@@ Expand Up / @@ -57,6 +57,9 @@ Configure the following settings:: @@
             # ...
         )
+        # The subdirectory of STATIC_ROOT that you wish to use for compiled bundles
+        BUNDLES_DIR = 'bundled'
         # This is the important part.
         MINIFY_BUNDLES = {
             'css': {},
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -70,7 +70,10 @@ def get_js_urls(bundle, debug=None): @@
             bundle_full = 'js:%s' % bundle
             if bundle_full in BUNDLE_HASHES:
                 build_id = BUNDLE_HASHES[bundle_full]
-            return (_get_item_path('js/%s-min.js?build=%s' % (bundle, build_id,)),)
+            return (_get_item_path(
+                os.path.join(
+                    getattr(settings, 'BUNDLES_DIR', ''),
+                    'js/%s-min.js?build=%s' % (bundle, build_id,))),)
     def _get_compiled_css_url(item):
@@ Expand Down Expand Up / @@ -120,8 +123,9 @@ def get_css_urls(bundle, debug=None): @@
             bundle_full = 'css:%s' % bundle
             if bundle_full in BUNDLE_HASHES:
                 build_id = BUNDLE_HASHES[bundle_full]
-            return (_get_item_path('css/%s-min.css?build=%s' %
-                                   (bundle, build_id)),)
+            item = 'css/%s-min.css?build=%s' % (bundle, build_id)
+            item = os.path.join(getattr(settings, 'BUNDLES_DIR', ''), item)
+            return (_get_item_path(item),)
     @register.function
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -32,10 +32,12 @@ class Command(BaseCommand): # pragma: no cover @@
         checked_hash = {}
         bundle_hashes = {}
+        tmp_files = []
         missing_files = 0
         minify_skipped = 0
         cmd_errors = False
         ext_media_path = os.path.join(get_media_root(), 'external')
+        bundles_dir = getattr(settings, 'BUNDLES_DIR', '.')
         def update_hashes(self, update=False):
             def media_git_id(media_path):
@@ Expand Down Expand Up / @@ -76,12 +78,17 @@ def handle(self, **options): @@
             for ftype, bundle in settings.MINIFY_BUNDLES.iteritems():
                 for name, files in bundle.iteritems():
                     # Set the paths to the files.
-                    concatted_file = path(ftype, '%s-all.%s' % (name, ftype,))
-                    compressed_file = path(ftype, '%s-min.%s' % (name, ftype,))
+                    concatted_file = path(self.bundles_dir, ftype,
+                                          '%s-all.%s' % (name, ftype,))
+                    compressed_file = path(self.bundles_dir, ftype,
+                                           '%s-min.%s' % (name, ftype,))
+                    if not os.path.exists(path(self.bundles_dir, ftype)):
+                        os.makedirs(path(self.bundles_dir, ftype))
                     files_all = []
                     for fn in files:
-                        processed = self._preprocess_file(fn)
+                        processed = self._preprocess_file(fn, compressed_file)
                         # If the file can't be processed, we skip it.
                         if processed is not None:
                             files_all.append(processed)
@@ Expand Down Expand Up / @@ -111,6 +118,16 @@ def handle(self, **options): @@
                     else:
                         self.minify_skipped += 1
+            # Clean up.
+            for file in self.tmp_files:
+                try:
+                    os.remove(path(file))
+                    # Try to remove the temporary directory (only removed if empty)
+                    os.rmdir(os.path.dirname(path(file)))
+                except OSError:
+                    pass
             # Write out the hashes
             self.update_hashes(options.get('add_timestamp', False))
@@ Expand All / @@ -134,11 +151,11 @@ def _get_url_or_path(self, item): @@
             """
             if item.startswith('//'):
                 return 'http:%s' % item
-            elif item.startswith(('http', 'https')):
+            elif item.startswith(('http:', 'https:')):
                 return item
             return None
-        def _preprocess_file(self, filename):
+        def _preprocess_file(self, filename, compressed_file):
             """Preprocess files and return new filenames."""
             url = self._get_url_or_path(filename)
             if url:
@@ Expand Down Expand Up / @@ -186,6 +203,12 @@ def _preprocess_file(self, filename): @@
                            (settings.STYLUS_BIN, os.path.dirname(fp), fp, fp),
                            shell=True, stdout=PIPE)
                 filename = '%s.css' % filename
+            if not url:
+                # Fix relative paths.
+                filename = self._fix_urls(filename, compressed_file)
+                self.tmp_files.append(filename)
             return path(filename.lstrip('/'))
         def _is_changed(self, concatted_file):
@@ Expand All / @@ -205,6 +228,38 @@ def _clean_tmp(self, concatted_file): @@
                 os.remove(concatted_file)
             os.rename(tmp_concatted, concatted_file)
+        def _fix_urls(self, filename, compressed_file):
+            """Fix the relative URLs in css files."""
+            css_content = ''
+            with open(path(filename)) as css_in:
+                css_content = css_in.read()
+            relpath = os.path.relpath(os.path.dirname(compressed_file),
+                                      path(os.path.dirname(filename)))
+            parse = lambda url: self._fix_urls_regex(url, relpath)
+            css_parsed = re.sub(r'url\(([^)]*?)\)', parse, css_content)
+            # Write to a temporary file.
+            out_file = path(os.path.join(self.bundles_dir, '.tmp',
+                                         '%s.tmp' % filename))
+            if not os.path.exists(os.path.dirname(out_file)):
+                os.makedirs(os.path.dirname(out_file))
+            with open(out_file, 'w') as css_out:
+                css_out.write(css_parsed)
+            return os.path.relpath(out_file, path('.'))
+        def _fix_urls_regex(self, url, relpath):
+            """Run over the regex; Fix relative URL's"""
+            url = url.group(1).strip('"\'')
+            if not url.startswith(('data:', 'http:', 'https:')):
+                url = os.path.relpath(url, relpath)
+            return 'url(%s)' % url
         def _cachebust(self, css_file, bundle_name):
             """Cache bust images.  Return a new bundle hash."""
             print "Cache busting images in %s" % re.sub('.tmp$', '', css_file)
@@ Expand Down Expand Up / @@ -268,7 +323,7 @@ def _file_hash(self, url): @@
         def _cachebust_regex(self, img, parent):
             """Run over the regex; img is the structural regex object."""
             url = img.group(1).strip('"\'')
-            if url.startswith('data:') or url.startswith('http'):
+            if url.startswith(('data:', 'http:', 'https:')):
                 return "url(%s)" % url
             url = url.split('?')[0]
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,11 +1,16 @@
+    import os
+    import tempfile
     from django.conf import settings
     from django.test.utils import override_settings
     import jingo
     from mock import ANY, call, patch
     from nose.tools import eq_
     from .utils import get_media_root, get_media_url
+    from .management.commands.compress_assets import Command
     try:
         from build import BUILD_ID_CSS, BUILD_ID_JS
@@ Expand Down Expand Up / @@ -66,8 +71,9 @@ def test_js_helper(getmtime, time): @@
         t = env.from_string("{{ js('common_protocol_less_url', debug=False) }}")
         s = t.render()
-        eq_(s, '<script src="%sjs/common_protocol_less_url-min.js?build=%s">'
-               '</script>' % (settings.STATIC_URL, BUILD_ID_JS))
+        eq_(s,
+            '<script src="%sjs/common_protocol_less_url-min.js?build=%s">'
+            '</script>' % (settings.STATIC_URL, BUILD_ID_JS))
         t = env.from_string("{{ js('common_bundle', debug=True) }}")
         s = t.render()
@@ Expand All / @@ -81,7 +87,7 @@ def test_js_helper(getmtime, time): @@
         s = t.render()
         eq_(s, '<script src="%sjs/common_bundle-min.js?build=%s"></script>' %
-               (settings.STATIC_URL, BUILD_ID_JS))
+            (settings.STATIC_URL, BUILD_ID_JS))
     @patch('jingo_minify.helpers.time.time')
@@ Expand Down Expand Up / @@ -163,6 +169,33 @@ def test_css_helper(getmtime, time): @@
                (settings.STATIC_URL, BUILD_ID_CSS))
+    @patch('jingo_minify.helpers.time.time')
+    @patch('jingo_minify.helpers.os.path.getmtime')
+    @override_settings(STATIC_ROOT='static',
+                       MEDIA_ROOT='media',
+                       STATIC_URL='http://example.com/static/',
+                       MEDIA_URL='http://example.com/media/',
+                       BUNDLES_DIR='bundled')
+    def test_bundles_dir(getmtime, time):
+        getmtime.return_value = 1
+        time.return_value = 1
+        env = jingo.env
+        t = env.from_string("{{ js('common', debug=False) }}")
+        s = t.render()
+        eq_(s, '<script src="%sbundled/js/common-min.js?build=%s"></script>' %
+               (settings.STATIC_URL, BUILD_ID_JS))
+        t = env.from_string("{{ css('common', debug=False) }}")
+        s = t.render()
+        eq_(s,
+            '<link rel="stylesheet" media="screen,projection,tv" '
+            'href="%sbundled/css/common-min.css?build=%s" />'
+            % (settings.STATIC_URL, BUILD_ID_CSS))
     def test_inline_css_helper():
         env = jingo.env
         t = env.from_string("{{ inline_css('common', debug=True) }}")
@@ Expand Down Expand Up / @@ -296,3 +329,72 @@ def test_js(getmtime, time): @@
              for j in settings.MINIFY_BUNDLES['js']['common']])
         eq_(s, expected)
+    @override_settings(STATIC_ROOT='static',
+                       MEDIA_ROOT='media',
+                       STATIC_URL='http://example.com/static/',
+                       MEDIA_URL='http://example.com/media/')
+    def test_fix_urls():
+        """Make sure relative URLs are fixed correctly."""
+        command = Command()
+        tmp = tempfile.mkstemp()[1]
+        tmp_file = open(tmp, 'w')
+        tmp_file.write('url(../img/test.png);')
+        tmp_file.close()
+        # Test files in same directory
+        compressed = command._fix_urls(tmp,
+            os.path.join(os.path.dirname(tmp), 'compiled'))
+        compressed_file = open(compressed, 'r')
+        eq_(compressed_file.read(), 'url(../img/test.png);')
+        compressed_file.close()
+        # Test files in different directory
+        compressed = command._fix_urls(tmp,
+            os.path.join(os.path.dirname(tmp), 'dir', 'compiled'))
+        compressed_file = open(compressed, 'r')
+        eq_(compressed_file.read(), 'url(../../img/test.png);')
+        compressed_file.close()
+        # Remove temporary files
+        os.remove(compressed)
+        os.remove(tmp)
+        # Test data urls are unchanged
+        data_url = 'url(data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulr);'
+        tmp = tempfile.mkstemp()[1]
+        tmp_file = open(tmp, 'w')
+        tmp_file.write(data_url)
+        tmp_file.close()
+        compressed = command._fix_urls(tmp,
+            os.path.join(os.path.dirname(tmp), 'compiled'))
+        compressed_file = open(compressed, 'r')
+        eq_(compressed_file.read(), data_url)
+        compressed_file.close()
+        # Remove temporary files
+        os.remove(compressed)
+        os.remove(tmp)
+        # Test absolute paths are unchanged
+        content = ('url(http://example.com/image.gif); '
+                   'url(https://example.com/image.png);')
+        tmp = tempfile.mkstemp()[1]
+        tmp_file = open(tmp, 'w')
+        tmp_file.write(content)
+        tmp_file.close()
+        compressed = command._fix_urls(tmp,
+            os.path.join(os.path.dirname(tmp), 'compiled'))
+        compressed_file = open(compressed, 'r')
+        eq_(compressed_file.read(), content)
+        compressed_file.close()
+        # Remove temporary files
+        os.remove(compressed)
+        os.remove(tmp)

-Original file line number
+Diff line change
@@ Expand Up / @@ -2,3 +2,4 @@ Django==1.4.5 @@
     jingo==0.4
     -e git://github.com/jbalogh/django-nose.git@83c7867c3f90ff3c7c7471716da91b643e8b2c01#egg=django_nose-dev
     mock==1.0b1
+    GitPython==0.1.7

Change bundle path and fix relative URL's #41

Are you sure you want to change the base?

Uh oh!

Change bundle path and fix relative URL's #41

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!