Asset#mtime was deprecated and not the correct value #197
Conversation
|
|
|
@fxn I'm not sure that http://ruby-doc.org/stdlib-2.2.3/libdoc/zlib/rdoc/Zlib/GzipWriter.html#method-i-mtime-3D
But it doesn't actually change the mtime of the file: Aside from that passing comment in the nginx docs you sent I don't know where the desired behavior is specified. Does it matter that the gzip file has a different mtime than the original file? Is this code doing the correct thing, but my test and my interpretations are wrong? |
|
I updated with a better test where we are verifying the header value, however i'm still not totally convinced that we are doing the right thing. Or even testing the right thing. I've not done extensive work with gzip files before this. |
|
Now that i'm looking closer i'm seeing some unrelated behavior that I don't quite understand. I'm removing the cache and public directory and generating assets: $ rm -rf public/assets/ tmp/cache/; time RAILS_ENV=production bundle exec rake assets:precompileSo all the mtimes should be on the same day (December 4) but they aren't: $ ls -la public/assets
total 5912
drwxr-xr-x 13 richardschneeman staff 442 Dec 4 10:45 .
drwxr-xr-x 9 richardschneeman staff 306 Dec 4 10:45 ..
-rw-r--r-- 1 richardschneeman staff 3007 Dec 4 10:45 .sprockets-manifest-41cc443057a47878dc92a852e992b981.json
-rw-r--r-- 1 richardschneeman staff 24521 Nov 30 12:14 application-4b2d42bd6b734668146fd1f2650d9e33d7d62e4e69916a668f1de6061c2c176a.css
-rw-r--r-- 1 richardschneeman staff 5018 Dec 4 10:45 application-4b2d42bd6b734668146fd1f2650d9e33d7d62e4e69916a668f1de6061c2c176a.css.gzNote that |
|
@schneems interesting! |
schneems
force-pushed
the
schneems/fix-mtime3
branch
from
3d7183e to
479ed6d
Compare
|
Isn't the most recent mtime of the files that composes application.css? On Fri, Dec 4, 2015, 14:55 Xavier Noria [email protected] wrote:
|
|
@rafaelfranca that would make sense. |
schneems
force-pushed
the
schneems/fix-mtime3
branch
from
479ed6d to
929d891
Compare
|
That kinda makes sense, I wonder why the mtime wouldn't just be when the file was written? Looks like the In 3.x this change comes here sprockets/lib/sprockets/asset.rb Line 170 in da13ee6 Back to original problemI would assume that setting the "header" mtime would mean that when you unzip the file it would have the mtime of we specified instead of when it was written too disk. But... Looking at We view the original mtimes November 30 is the mtime we are shooting for. Delete the css file and unzip css.gz file: We are still seeing a December 4th mtime, so even if we are setting the mtime header correctly, it doesn't do what we want. I wasn't sure if we were setting the correct value. I added some debug statements to verify we are setting the right mtime inside of I can manually set the mtime of the gzipped file: gz = Zlib::GzipWriter.new(f, Zlib::BEST_COMPRESSION)
puts target.inspect
gz.mtime = mtime
gz.write(@source)
gz.close
File.utime(mtime, mtime, f.path)Then I get the right output: and So either the |
|
I looked on http://www.zlib.net/ and none of the specifications mention using the |
This change pulls mtime from stat instead of relying on the asset object. Reference: 16ccffc#commitcomment-14779288 We don't need to use the environment stat cache because this file has likely never been stat-ed before. We also need to explicitly set the mtime of the file since it appears setting the mtime header has no impact on the mtime of the decompressed file.
schneems
force-pushed
the
schneems/fix-mtime3
branch
from
929d891 to
f5a9dde
Compare
In the case where a deploy was happening after all the assets were already generated, a future to write a file to disk is not created, (i.e. in the "skipped writing" file code path). We now check for the presence of this future before trying to `wait!` on it.
Asset#mtime was deprecated and not the correct value
See #197 (comment) for more information.
See #197 (comment) for more information.
|
I got a response from the zlib group. So the header is used in some cases as anticipated but it might not be the default behavior. I think the correct thing to do based on this information and the nginx docs is to set the mtime of the file and the header to the same value which matches the original file. This is what we are currently doing. |
|
Awesome! |
This was discussed in rails/sprockets#197. The behavior and results of setting mtime via this method were unclear. This document should help clarify the behavior.
This change pulls mtime from the environment stat cache instead of relying on the asset object. Reference: 16ccffc#commitcomment-14779288