node
node copied to clipboard
nodejs
[v16.x backport] fs: use signed types for stat data
Backport of: https://github.com/nodejs/node/pull/43714
On arm-12+, fsPromises.stat timestamps are returned as 0 even for small positive values. Excluding this platform from test.
On
arm-12+,fsPromises.stattimestamps are returned as 0 even for small positive values. Excluding this platform from test.
Doesn't that mean that 9e40873d4281b91fa10aacdad1b3f3326f6f00cb...5311b7d54260a03def6d65215e7ec442be55c4e1 should be fast-tracked on main first?
On
arm-12+,fsPromises.stattimestamps are returned as 0 even for small positive values. Excluding this platform from test.Doesn't that mean that 9e40873...5311b7d should be fast-tracked on
mainfirst?
FWIW as background arm-12+ is referring to https://ci.nodejs.org/job/node-test-binary-arm-12+/ which is the job that runs tests for armv7l on Raspberry Pi devices. They currently aren't being run on main/v18.x as they need updates which is why the failures haven't been seen until the tests were ported here to v16 (where we're still running tests on the Pi's).
Also FWIW I suspect the issue isn't actually Linux on arm but the filesystem being used.
Doesn't that mean that 9e40873...5311b7d should be fast-tracked on
mainfirst?
Yes, if similar test environment will be used on newer versions as well.
FWIW as background arm-12+ is referring to https://ci.nodejs.org/job/node-test-binary-arm-12+/ which is the job that runs tests for armv7l on Raspberry Pi devices. They currently aren't being run on main/v18.x as they need updates which is why the failures haven't been seen until the tests were ported here to v16 (where we're still running tests on the Pi's).
Also FWIW I suspect the issue isn't actually Linux on arm but the filesystem being used.
Thanks for clarification! Which FS is used on that test platform? Is there a straightforward way to detect it? Othewise we can add something like
await fsPromises.utimes(filepath, 1000, 1000);
const { atimeMs, mtimeMs } = await fsPromises.stat(filepath);
if (atimeMs !== 1000 || mtimeMs !== 1000) common.skip('bad platform or fs');
But it still feels weird to literally skip a test if it fails. 😅
Doesn't that mean that 9e40873...5311b7d should be fast-tracked on
mainfirst?Yes, if similar test environment will be used on newer versions as well.
I disagree, it's not because this project doesn't have a CI for this environment that it's not worth landing it on main. If someone with this env wants to compile node and run the test themself, there's no reason we should impose them a false positive test failure.
FWIW as background arm-12+ is referring to https://ci.nodejs.org/job/node-test-binary-arm-12+/ which is the job that runs tests for armv7l on Raspberry Pi devices. They currently aren't being run on main/v18.x as they need updates which is why the failures haven't been seen until the tests were ported here to v16 (where we're still running tests on the Pi's). Also FWIW I suspect the issue isn't actually Linux on arm but the filesystem being used.
Thanks for clarification! Which FS is used on that test platform? Is there a straightforward way to detect it?
They are NFS mounts, e.g.
$ df -Th /home/iojs/build/workspace/node-test-binary-arm/
Filesystem Type Size Used Avail Use% Mounted on
192.168.1.10:/exports/nodejs/pi/test-requireio_williamkapke-debian10-arm64_pi3-3 nfs 458G 206G 230G 48% /
$
On the Pi's the tests are run in containers and folder above is mounted inside the container on the machine.
If I understand correctly, after https://github.com/nodejs/node/pull/44174 lands, we can close this PR and remove the backport label from https://github.com/nodejs/node/pull/43714 ?
I disagree, it's not because this project doesn't have a CI for this environment that it's not worth landing it on
main. If someone with this env wants to compile node and run the test themself, there's no reason we should impose them a false positive test failure.
Perhaps I worded it incorrectly: test env was related to fast-tracking, i.e. if it should be ported to main ASAP.
As for local user environments, I think it would be reasonable to keep the test as strict as possible, as long as it doesn't break common cases (example: absence of ipv6 support is common, blocked ports for localhost are not), and doesn't unconditionally break CI.
If there is a better approach for such tests, I'm open for suggestions.
They are NFS mounts
I guess NFS might not support atime? On both first and second tries it returned 0, so it doesn't look like fsync-related issue.
If I understand correctly, after #44174 lands, we can close this PR and remove the backport label from #43714 ?
Yes. If #43714 with #44174 will be mergeable into v16 without any problems, the label can be removed and this PR can be closed.