doc: note that fs.futimes only works on AIX >7.1 by gibfahn · Pull Request #13659 · nodejs/node

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

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

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

doc/api/fs.md

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

Nit: *Note*:

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

doc/api/fs.md

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

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 <gpunathi@in.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> i

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

Landed in 4eeb4a4

PR-URL: #13659 Fixes: #12609 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> 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

doc/api/fs.md Outdated

Copy link

Contributor

refack Jun 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Uh oh!

Conversation

gibfahn commented Jun 13, 2017

Checklist

Affected core subsystem(s)

Uh oh!

gibfahn commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jun 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardlau commented Jun 15, 2017

Uh oh!

gibfahn commented Jun 16, 2017

Uh oh!

refack commented Jun 28, 2017

Uh oh!

jasnell commented Jun 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

gibfahn Jun 16, 2017 •

edited

Loading