doc: note that fs.futimes only works on AIX >7.1 #13659

gibfahn · 2017-06-13T13:48:58Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

doc

cc/ @nodejs/platform-aix @gireeshpunathil

gibfahn · 2017-06-13T13:51:49Z

cc/ @aqrln @refack @jasnell @Trott re. #13133, what do you think of this for an AIX-specific note? I went with *Note (AIX):*

refack · 2017-06-13T16:09:44Z

doc/api/fs.md

@@ -1225,6 +1225,9 @@ changes:
 Change the file timestamps of a file referenced by the supplied file
 descriptor.

+*Note (AIX):* This function only works for AIX 7.1 and newer. It can still be


Maybe *Note*: AIX - ?
Re: #13133

jasnell · 2017-06-13T16:13:13Z

doc/api/fs.md

@@ -1225,6 +1225,9 @@ changes:
 Change the file timestamps of a file referenced by the supplied file
 descriptor.

+*Note (AIX):* This function only works for AIX 7.1 and newer. It can still be
+called on older versions but will return `UV_ENOSYS`.
+


I would just make it *Note*: given that the text clearly states AIX already.

Or perhaps,

*Note*: For AIX systems, this function only works on 7.1 or newer. On older AIX versions, calling this function will always return `UV_ENOSYS`.

I would keep 'AIX' in front of '7.1' to make it clear this refers to the OS version and not the Node.js version.

Updated, I think it should be unambiguous now.

refack · 2017-06-14T12:27:46Z

doc/api/fs.md

@@ -1225,6 +1225,9 @@ changes:
 Change the file timestamps of a file referenced by the supplied file
 descriptor.

+*Note:* This function only works for AIX 7.1 and newer. It can still be


Nit: *Note*:

richardlau · 2017-06-14T14:31:08Z

doc/api/fs.md

@@ -1225,6 +1225,9 @@ changes:
 Change the file timestamps of a file referenced by the supplied file
 descriptor.

+*Note*: This function only works for AIX 7.1 and newer. It can still be


Now it sounds like it doesn't work on non-AIX platforms.

Suggestions welcome. I don't want it to get excessively long.

And I think the second sentence makes it pretty clear that this is an AIX specific issue.

*Note*: On AIX systems, this function only works for versions 7.1 and newer...

gibfahn · 2017-06-14T20:09:17Z

@richardlau @jasnell reversed it to (hopefully) preserve clarity while maintaining brevity, PTAL.

mhdawson · 2017-06-15T06:09:56Z

doc/api/fs.md

@@ -1225,6 +1225,9 @@ changes:
 Change the file timestamps of a file referenced by the supplied file
 descriptor.

+*Note*: This function does not work on AIX versions older than 7.1. It can still
+be called but will return `UV_ENOSYS`.
+


This does not sound right. The earlier description said it only worked on version 7.1 and newer, now it says it no longer works after 7.1

Are you saying that older for you means later?

it only worked on version 7.1 and newer

does not work on AIX versions older than 7.1

These two phrases seem identical to me.

Bear with us @gibfahn, the bike shed is not quite yet the right shade ;-) ...

*Note*: This function does not work correctly on AIX versions older than 7.1 in that it will always return the value `UV_ENOSYS`.

I like the idea of linking the two sentences, but that suggests that UV_ENOSYS is a reasonable thing for it to return. How about:

*Note*: This function does not work on AIX versions older than 7.1, it will return the error `UV_ENOSYS`.

How about:

Note: This function does not work on AIX versions before 7.1,
it will return the error UV_ENOSYS.

While I agree older probably means the right thing the numbering is opposite to what usually comes to mind with older (ie 72 is older than 71 in years)

Hmm, let's see how it looks with js formatting:

*Note*: This function does not work on AIX versions before 7.1, it will return the error UV_ENOSYS.

Yep, looks good to me.

I like this shade for the shed...

and it will return with error code UV_ENOSYS.

_{(with go coloring)}

richardlau · 2017-06-15T19:56:32Z

Given that since Node.js 8 we don't support AIX versions earlier than 7.1 TL04 (https://github.com/nodejs/node/blob/v8.0.0/BUILDING.md), do we need to document this limitation on 8 and master?

Definitely needs to be documented on the earlier Node.js release lines.

Fixes: nodejs#12609

gibfahn · 2017-06-16T11:37:36Z

@richardlau @mhdawson LGTY?

refack · 2017-06-28T23:14:09Z

@gibfahn land before the lighting changes and everyone wants a new color again.

PR-URL: #13659 Fixes: #12609 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> i

jasnell · 2017-06-29T03:17:57Z

Landed in 4eeb4a4

PR-URL: #13659 Fixes: #12609 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> i

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jun 13, 2017

gibfahn added the aix Issues and PRs related to the AIX platform. label Jun 13, 2017

gireeshpunathil approved these changes Jun 13, 2017

View reviewed changes

refack approved these changes Jun 13, 2017

View reviewed changes

jasnell reviewed Jun 13, 2017

View reviewed changes

cjihrig approved these changes Jun 13, 2017

View reviewed changes

jasnell approved these changes Jun 13, 2017

View reviewed changes

gibfahn force-pushed the futimes-aix branch from 3bbc3ad to 304b555 Compare June 14, 2017 09:25

refack approved these changes Jun 14, 2017

View reviewed changes

refack reviewed Jun 14, 2017

View reviewed changes

gibfahn force-pushed the futimes-aix branch from 304b555 to 5a6c395 Compare June 14, 2017 12:34

refack approved these changes Jun 14, 2017

View reviewed changes

richardlau reviewed Jun 14, 2017

View reviewed changes

gibfahn force-pushed the futimes-aix branch 2 times, most recently from e7c2a0d to 9dae270 Compare June 14, 2017 20:08

mhdawson reviewed Jun 15, 2017

View reviewed changes

doc: note that fs.futimes only works on AIX >7.1

053ab6b

Fixes: nodejs#12609

gibfahn force-pushed the futimes-aix branch from 9dae270 to 053ab6b Compare June 16, 2017 11:36

richardlau approved these changes Jun 16, 2017

View reviewed changes

mhdawson approved these changes Jun 20, 2017

View reviewed changes

gireeshpunathil mentioned this pull request Jun 21, 2017

Document that fs.futimes isn't supported on AIX #12609

Closed

jasnell closed this Jun 29, 2017

gibfahn deleted the futimes-aix branch June 29, 2017 07:42

addaleax mentioned this pull request Jun 29, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the land-on-v6.x label Aug 14, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: note that fs.futimes only works on AIX >7.1 #13659

doc: note that fs.futimes only works on AIX >7.1 #13659

gibfahn commented Jun 13, 2017

gibfahn commented Jun 13, 2017

refack Jun 13, 2017

jasnell Jun 13, 2017

jasnell Jun 13, 2017

richardlau Jun 13, 2017

gibfahn Jun 14, 2017

refack Jun 14, 2017

gibfahn Jun 14, 2017

richardlau Jun 14, 2017

gibfahn Jun 14, 2017

gibfahn Jun 14, 2017

jasnell Jun 14, 2017

gibfahn commented Jun 14, 2017

mhdawson Jun 15, 2017

gibfahn Jun 15, 2017

jasnell Jun 15, 2017

gibfahn Jun 15, 2017

mhdawson Jun 16, 2017

gibfahn Jun 16, 2017 •

edited

Loading

refack Jun 16, 2017

richardlau commented Jun 15, 2017

gibfahn commented Jun 16, 2017

refack commented Jun 28, 2017

jasnell commented Jun 29, 2017

doc: note that fs.futimes only works on AIX >7.1 #13659

doc: note that fs.futimes only works on AIX >7.1 #13659

Conversation

gibfahn commented Jun 13, 2017

Checklist

Affected core subsystem(s)

gibfahn commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau commented Jun 15, 2017

gibfahn commented Jun 16, 2017

refack commented Jun 28, 2017

jasnell commented Jun 29, 2017

gibfahn Jun 16, 2017 •

edited

Loading