fix syncing utimes if copied files #268

nalajcie · 2016-02-01T14:08:57Z

fs.utimesSync saves times with one second precision, while fs.statSync gives us microsecond precision.

This makes utimes not synchronized on filesystems with sub-second atime/ctime/mtime precision.

Upgraded to use newer fs.futimesSync interface providing correct precision.

Fixes #257.

XhmikosR · 2016-02-01T16:20:48Z

Doesn't seem to work on Windows.


> [email protected] test C:\Users\xmr\Desktop\grunt\grunt-contrib-copy
> grunt test

Running "jshint:all" (jshint) task
>> 3 files lint free.

Running "clean:test" (clean) task
>> 1 path cleaned.

Running "copy:main" (copy) task
Warning: EPERM: operation not permitted, futime Use --force to continue.

Aborted due to warnings.
npm ERR! Test failed.  See above for more details.

C:\Users\xmr\Desktop\grunt\grunt-contrib-copy>

nalajcie · 2016-02-01T22:53:21Z

Fixed file open mode for windows platform (it requires 'r+' mode to be able to change utimes, linux requires dirs to be opened with 'r' only).

Unfortunately, the tests will not work on windows platform:

chmod on windows is not working as expected (detailed comment in the source: https://github.com/nodejs/node/blob/master/deps/uv/src/win/fs.c#L1064) - probably the test should be disabled or changed to be less restrictive on windows platform
timestamp will not have sub-second precision due to bug in nodejs (issue: Windows: Setting fs.futimes{Sync} subsecond (millisecond) precision? nodejs/node#2069)

XhmikosR · 2016-02-06T07:23:44Z

We could skip the test on Windows as long as things work even without the subsecond precision.

sindresorhus · 2016-02-07T05:49:54Z

tasks/copy.js

@@ -14,6 +14,7 @@ module.exports = function(grunt) {
  var fs = require('fs');
  var chalk = require('chalk');
  var fileSyncCmp = require('file-sync-cmp');
+  var isWindows = /^win/.test(process.platform);


Should be: var isWindows = process.platform === 'win32'; And yes, it's win32 for 64-bit too.

XhmikosR · 2016-02-23T22:41:24Z

@shama @vladikoff: any ideas how we should proceed with this?

shama · 2016-02-23T23:57:39Z

I'm good with skipping that test on Windows for now and merging this.

XhmikosR · 2016-02-24T17:01:31Z

@nalajcie: can you rebase and squash the patches? Maybe leave nalajcie@99dded8 as a separate one though.

fs.utimesSync saves times with one second precision, while fs.statSync gives us microsecond precision. This makes utimes not synchronized on filesystems with sub-second atime/ctime/mtime precision. Upgraded to use newer fs.futimesSync interface providing correct precision.

This test will not pass until it would be fixed in nodejs See: nodejs/node#2069

fix syncing utimes if copied files

XhmikosR · 2016-02-25T06:01:24Z

Thanks!

krnlde · 2016-03-14T12:04:48Z

So the issue on windows will persist?

shama · 2016-03-14T15:38:27Z

@krnlde This is an issue with our testing on Windows. Are you running into a syncing utime issue on Windows?

krnlde · 2016-03-14T16:21:48Z

Yes. I updated to 1.0.0 today but that didn't help. There are also a few tickets in grunt newer. They are not sure where it comes from. And so am I.

For the record:

Running 'grunt newer:jshint' causes the full jshint task to run tschaub/grunt-newer#39
Simply doesn't work with copy task tschaub/grunt-newer#84
Does not work with grunt-contrib-copy due to the timestamp precision disagreement tschaub/grunt-newer#86

tschaub · 2016-03-15T16:46:27Z

@krnlde if you can provide a repo that includes the minimum needed to reproduce your problem, that would be helpful.

I tried https://github.com/giovannipds/grunt-newer-copy-test and this works as expected on OS X with [email protected]. I'd be interested to know if others can reproduce the problem with that repo outside Windows.

It would also be good to know if the changes in [email protected] make a difference on Windows.

krnlde · 2016-04-14T11:21:18Z

Hey @tschaub sorry I missed your response. As stated in tschaub/grunt-newer#86 it should not be possible to reproduce this outside Windows. They (you) implemented a tolerance option to workaround this problem, but a definitive solution would of course be better.

Regarding the repo to reproduce the behavior (on Windows): we are using it in https://github.com/micromata/bootstrap-kickstart . Unfortunately this is not the minimum needed setup you requested. But I don't have time to create one right now. Does this work for you anyway?

simevo · 2017-05-02T09:15:51Z

Hi, while testing this module as part of the debian packaging effort, we noticed timestamp failures on i386 and armhf architectures. This is on:

Linux with kernel 4.9.0
ext4 filesystem
nodejs v4.8.2

For more details see: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=861515
Is there anything we should try to pinpoint the issue ?

XhmikosR · 2017-05-02T09:22:36Z

@simevo: the issue shouldn't happen in the latest release. So if it still happens, there must be a difference in how the OS handles times. The main change in this PR is 217819f#diff-951068604e1c942635e6a1f96a88994dL122

simevo · 2017-05-02T10:19:43Z

We're on grunt-contrib-copy v1.0.0 and we have the commit you mention. I have reproduced the issue on a i386 lxc container running debian sid within a amd64 debian host running strecth, as follows:

apt install wget xz-utils git
cd /opt
wget https://nodejs.org/download/release/v4.8.2/node-v4.8.2-linux-x86.tar.xz
xz -d node-v4.8.2-linux-x86.tar.xz
tar xf node-v4.8.2-linux-x86.tar
rm node-v4.8.2-linux-x86.tar
echo 'export PATH=/opt/node-v4.8.2-linux-x86/bin:$PATH' >> ~/.bashrc
source ~/.bashrc
cd
git clone https://github.com/gruntjs/grunt-contrib-copy.git
cd grunt-contrib-copy
npm install grunt chalk file-sync-cmp jshint grunt-contrib-jshint grunt-contrib-clean grunt-contrib-nodeunit grunt-contrib-internal
grunt test

this is what I get:

Running "nodeunit:tests" (nodeunit) task
Testing copy_test.js.......F.
>> copy - timestampEqual
>> Error: 1493719861389 == 1493719861494
>>   at Object.exports.copy.timestampEqual (test/copy_test.js:92:10)
>>   at processImmediate [as _immediateCallback] (timers.js:396:17)

>> copy - timestampEqual
>> Error: 1493719861389 == 1493719861494
>>   at Object.exports.copy.timestampEqual (test/copy_test.js:93:10)
>>   at processImmediate [as _immediateCallback] (timers.js:396:17)

Warning: 2/17 assertions failed (19ms) Use --force to continue.

additional info:

mount | grep ' on / '
/dev/sda1 on / type btrfs (rw,relatime,ssd,space_cache,subvolid=2373,subvol=/var/lib/lxc/debian10_32_packaging/rootfs)

uname -a
Linux debian10_32_packaging 4.9.0-2-amd64 #1 SMP Debian 4.9.18-1 (2017-03-30) i686 GNU/Linux

node -v
v4.8.2

XhmikosR · 2017-05-02T10:57:31Z

What happens with a newer node version?

…

On May 2, 2017 13:19, "simevo" ***@***.***> wrote: We're on grunt-contrib-copy v1.0.0 and we have the commit you mention. I have reproduced the issue on a i386 lxc container running debian sid within a amd64 debian host running strecth, as follows: apt install wget xz-utils git cd /opt wget https://nodejs.org/download/release/v4.8.2/node-v4.8.2-linux-x86.tar.xz xz -d node-v4.8.2-linux-x86.tar.xz tar xf node-v4.8.2-linux-x86.tar rm node-v4.8.2-linux-x86.tar echo 'export PATH=/opt/node-v4.8.2-linux-x86/bin:$PATH' >> ~/.bashrc source ~/.bashrc cd git clone https://github.com/gruntjs/grunt-contrib-copy.git cd grunt-contrib-copy npm install grunt chalk file-sync-cmp jshint grunt-contrib-jshint grunt-contrib-clean grunt-contrib-nodeunit grunt-contrib-internal grunt test this is what I get: Running "nodeunit:tests" (nodeunit) task Testing copy_test.js.......F. >> copy - timestampEqual >> Error: 1493719861389 == 1493719861494 >> at Object.exports.copy.timestampEqual (test/copy_test.js:92:10) >> at processImmediate [as _immediateCallback] (timers.js:396:17) >> copy - timestampEqual >> Error: 1493719861389 == 1493719861494 >> at Object.exports.copy.timestampEqual (test/copy_test.js:93:10) >> at processImmediate [as _immediateCallback] (timers.js:396:17) Warning: 2/17 assertions failed (19ms) Use --force to continue. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#268 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVVtZYH0fVbS3kGagE7OFygbAodY_xpks5r1wNAgaJpZM4HQkPq> .

simevo · 2017-05-02T11:10:53Z

node -v
v6.10.2

I tear down the grunt-contrib-copy dir and pull everything again, then node_modules/grunt/bin/grunt test returns:

...
Running "nodeunit:tests" (nodeunit) task
Testing copy_test.js.......F.
>> copy - timestampEqual
>> Error: 1493723278009 == 1493723278082
>>   at Object.timestampEqual (test/copy_test.js:92:10)
>>   at runCallback (timers.js:666:20)
>>   at tryOnImmediate (timers.js:639:5)
>>   at processImmediate [as _immediateCallback] (timers.js:611:5)

>> copy - timestampEqual
>> Error: 1493723278009 == 1493723278082
>>   at Object.timestampEqual (test/copy_test.js:93:10)
>>   at runCallback (timers.js:666:20)
>>   at tryOnImmediate (timers.js:639:5)
>>   at processImmediate [as _immediateCallback] (timers.js:611:5)

Warning: 2/17 assertions failed (19ms) Use --force to continue.

XhmikosR · 2017-05-02T12:18:01Z

Then only the OS is left as a variable here. Not sure what else can be done.

simevo · 2017-05-02T21:40:15Z

To make sure it's not an artifact of lxc or a side-effect of mixing amd64 host (and kernel) with i386 libc &C, I set up an i386 VirtualBox clone. It is reproducible there too. Should I open an issue on this ?

XhmikosR · 2017-05-02T21:42:56Z

If it's something you can consistently reproduce with a clean OS which is the latest stable version, then sure. Weird that Travis CI is passing though.

simevo · 2017-05-03T05:54:56Z

done ! travis is running on arch: "amd64" (64-bit), we're seeing this on i386 (32-bit)

nalajcie mentioned this pull request Feb 1, 2016

Does not work with grunt-newer due to the timestamp precision disagreement #257

Closed

XhmikosR added the bug label Feb 1, 2016

nalajcie mentioned this pull request Feb 2, 2016

Unit tests failing on Node v0.12 #216

Closed

sindresorhus reviewed Feb 7, 2016
View reviewed changes

nalajcie and others added 2 commits February 24, 2016 20:59

tests: disable testing timestamp equity under windows

147a84b

This test will not pass until it would be fixed in nodejs See: nodejs/node#2069

nalajcie force-pushed the master branch from 5473311 to 147a84b Compare February 24, 2016 21:05

XhmikosR added a commit that referenced this pull request Feb 25, 2016

Merge pull request #268 from nalajcie/master

829032d

fix syncing utimes if copied files

XhmikosR merged commit 829032d into gruntjs:master Feb 25, 2016

simevo mentioned this pull request May 3, 2017

timestampEqual test fails on i386 Linux #291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix syncing utimes if copied files #268

fix syncing utimes if copied files #268

nalajcie commented Feb 1, 2016

XhmikosR commented Feb 1, 2016

nalajcie commented Feb 1, 2016

XhmikosR commented Feb 6, 2016

sindresorhus Feb 7, 2016

XhmikosR commented Feb 23, 2016

shama commented Feb 23, 2016

XhmikosR commented Feb 24, 2016

XhmikosR commented Feb 25, 2016

krnlde commented Mar 14, 2016

shama commented Mar 14, 2016

krnlde commented Mar 14, 2016

tschaub commented Mar 15, 2016

krnlde commented Apr 14, 2016

simevo commented May 2, 2017 •

edited

Loading

XhmikosR commented May 2, 2017

simevo commented May 2, 2017 •

edited

Loading

XhmikosR commented May 2, 2017 via email

simevo commented May 2, 2017

XhmikosR commented May 2, 2017

simevo commented May 2, 2017

XhmikosR commented May 2, 2017

simevo commented May 3, 2017

fix syncing utimes if copied files #268

fix syncing utimes if copied files #268

Conversation

nalajcie commented Feb 1, 2016

XhmikosR commented Feb 1, 2016

nalajcie commented Feb 1, 2016

XhmikosR commented Feb 6, 2016

sindresorhus Feb 7, 2016

Choose a reason for hiding this comment

XhmikosR commented Feb 23, 2016

shama commented Feb 23, 2016

XhmikosR commented Feb 24, 2016

XhmikosR commented Feb 25, 2016

krnlde commented Mar 14, 2016

shama commented Mar 14, 2016

krnlde commented Mar 14, 2016

tschaub commented Mar 15, 2016

krnlde commented Apr 14, 2016

simevo commented May 2, 2017 • edited Loading

XhmikosR commented May 2, 2017

simevo commented May 2, 2017 • edited Loading

XhmikosR commented May 2, 2017 via email

simevo commented May 2, 2017

XhmikosR commented May 2, 2017

simevo commented May 2, 2017

XhmikosR commented May 2, 2017

simevo commented May 3, 2017

simevo commented May 2, 2017 •

edited

Loading

simevo commented May 2, 2017 •

edited

Loading