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

Preserve original mtime when copying static files #3220

Merged
merged 1 commit into from Jan 14, 2015

Conversation

Projects
None yet
6 participants
@inukshuk
Contributor

inukshuk commented Dec 19, 2014

Use the preserve option when copying static files using FileUtils.cp; this will preserve the file's original mtime. To preserve the original mtime of a file is important, for example, when syncing the _site folder.

@inukshuk

This comment has been minimized.

Show comment
Hide comment
@inukshuk

inukshuk Dec 19, 2014

Contributor

Sorry, I missed that a test guards against this change -- is there a reason why you would want the mtimes to change even if the original file is identical?

I'd be happy to update the test as well in case you'd be willing to accept this PR.

Contributor

inukshuk commented Dec 19, 2014

Sorry, I missed that a test guards against this change -- is there a reason why you would want the mtimes to change even if the original file is identical?

I'd be happy to update the test as well in case you'd be willing to accept this PR.

@chrisfinazzo

This comment has been minimized.

Show comment
Hide comment
@chrisfinazzo

chrisfinazzo Dec 22, 2014

Contributor

As far as I know, this is still an issue with Jekyll and FTP, so rsync is the way to go if you possibly can (See #1544)

That said, it's been a long time since I actually looked closely at my server to see what the current behavior is. Mostly I've just followed Nathan Grigg's advice for syncing Jekyll, which seems to work and is pretty fast.

Contributor

chrisfinazzo commented Dec 22, 2014

As far as I know, this is still an issue with Jekyll and FTP, so rsync is the way to go if you possibly can (See #1544)

That said, it's been a long time since I actually looked closely at my server to see what the current behavior is. Mostly I've just followed Nathan Grigg's advice for syncing Jekyll, which seems to work and is pretty fast.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 23, 2014

Member

Previous work: #370, #227. If this is merely for static files, I think it'd be fine. What relies on a new mtime if the file is the same? If you can use rsync, that would certainly be ideal (and likely faster than SFTP). Let's let this brew for a bit.

Member

parkr commented Dec 23, 2014

Previous work: #370, #227. If this is merely for static files, I think it'd be fine. What relies on a new mtime if the file is the same? If you can use rsync, that would certainly be ideal (and likely faster than SFTP). Let's let this brew for a bit.

@inukshuk

This comment has been minimized.

Show comment
Hide comment
@inukshuk

inukshuk Dec 24, 2014

Contributor

I'm actually using StaticFile from a plugin where I do some extensive image manipulation using a custom cache to speed up builds if nothing has changed and some other gimmicks. I just wanted to make sure that the build process uses the files from my custom cache and that's where I noticed that mtimes were being updated; it's no big deal in my case, since I need to patch StaticFile anyway, but I just assumed it would make sense to leave the timestamps of static files intact. Obvioulsy, generated files are a much trickier issue.

I'll gladly update the tests in a meaningful way if you decide to green light this, just let me know.

Contributor

inukshuk commented Dec 24, 2014

I'm actually using StaticFile from a plugin where I do some extensive image manipulation using a custom cache to speed up builds if nothing has changed and some other gimmicks. I just wanted to make sure that the build process uses the files from my custom cache and that's where I noticed that mtimes were being updated; it's no big deal in my case, since I need to patch StaticFile anyway, but I just assumed it would make sense to leave the timestamps of static files intact. Obvioulsy, generated files are a much trickier issue.

I'll gladly update the tests in a meaningful way if you decide to green light this, just let me know.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jan 2, 2015

Member

I'm in favor of preserving mtimes when copying static files from the source to the destination directory.

Member

mattr- commented Jan 2, 2015

I'm in favor of preserving mtimes when copying static files from the source to the destination directory.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 2, 2015

Member

I can get onboard with this. Looks like preserve also copies all the other metadata. Are we cool with this, too? Worried it might create permissions issues.

Member

parkr commented Jan 2, 2015

I can get onboard with this. Looks like preserve also copies all the other metadata. Are we cool with this, too? Worried it might create permissions issues.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jan 2, 2015

Member

@parkr you're right, preserve is too much for what we want.

Looks like we'll need special logic for handling mtimes.

Member

mattr- commented Jan 2, 2015

@parkr you're right, preserve is too much for what we want.

Looks like we'll need special logic for handling mtimes.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 2, 2015

Member

Looks like we'll need special logic for handling mtimes.

Something like https://github.com/jekyll/jekyll/pull/370/files#diff-17fc003a02cf3a2fc94e8a51d85f3f4fR215 ?

Member

parkr commented Jan 2, 2015

Looks like we'll need special logic for handling mtimes.

Something like https://github.com/jekyll/jekyll/pull/370/files#diff-17fc003a02cf3a2fc94e8a51d85f3f4fR215 ?

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jan 2, 2015

Member

Yup. That should do the trick. 😃

Member

mattr- commented Jan 2, 2015

Yup. That should do the trick. 😃

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2015

Member

Please apply this & fix the tests:

diff --git a/lib/jekyll/static_file.rb b/lib/jekyll/static_file.rb
index 6d3c6d2..37f2f9f 100644
--- a/lib/jekyll/static_file.rb
+++ b/lib/jekyll/static_file.rb
@@ -80,7 +80,8 @@ module Jekyll

       FileUtils.mkdir_p(File.dirname(dest_path))
       FileUtils.rm(dest_path) if File.exist?(dest_path)
-      FileUtils.cp(path, dest_path, :preserve => true)
+      FileUtils.cp(path, dest_path)
+      File.utime(@@mtimes[path], @@mtimes[path], dest_path)

       true
     end
Member

parkr commented Jan 10, 2015

Please apply this & fix the tests:

diff --git a/lib/jekyll/static_file.rb b/lib/jekyll/static_file.rb
index 6d3c6d2..37f2f9f 100644
--- a/lib/jekyll/static_file.rb
+++ b/lib/jekyll/static_file.rb
@@ -80,7 +80,8 @@ module Jekyll

       FileUtils.mkdir_p(File.dirname(dest_path))
       FileUtils.rm(dest_path) if File.exist?(dest_path)
-      FileUtils.cp(path, dest_path, :preserve => true)
+      FileUtils.cp(path, dest_path)
+      File.utime(@@mtimes[path], @@mtimes[path], dest_path)

       true
     end
@inukshuk

This comment has been minimized.

Show comment
Hide comment
@inukshuk

inukshuk Jan 12, 2015

Contributor

I have patched and rebased the PR; also updated the test to test that file is regenerated with the same mtime (after being deleted). Please let me know if you would like me to change anything!

Contributor

inukshuk commented Jan 12, 2015

I have patched and rebased the PR; also updated the test to test that file is regenerated with the same mtime (after being deleted). Please let me know if you would like me to change anything!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 12, 2015

Member

LGTM. /cc @jekyll/core.

Member

parkr commented Jan 12, 2015

LGTM. /cc @jekyll/core.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jan 13, 2015

Member

👍

Member

alfredxing commented Jan 13, 2015

👍

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 13, 2015

Member

@mattr- @envygeeks Any objections?

Member

parkr commented Jan 13, 2015

@mattr- @envygeeks Any objections?

mattr- added a commit that referenced this pull request Jan 14, 2015

@mattr- mattr- merged commit ce78ea7 into jekyll:master Jan 14, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Jan 14, 2015

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 14, 2015

Member

❤️

Member

parkr commented Jan 14, 2015

❤️

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.