GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||
GeezerVison is showing github.com — fs: re-enable watch facility in AIX by gireeshpunathil · Pull Request #10085 · nodejs/node Conversation
|
|
I am excited to see the AIX exceptions removed from parallel.status. Thanks for doing this! The JS test code change looks good to me. The text in the paragraph added to the doc needs some editing. Maybe someone from @nodejs/documentation can take a look and provide a reformatted version. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Oh, I see there are changes in |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
No. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@Trott , @bnoordhuis , thanks. So what is the recommendation here? first have a PR for the uv change, and then come back here with the rest of the changes? or progress this PMR and schedule the uv PR before the next libuv release separately? sorry for being blunt here, I don't know the process much. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@gireeshpunathil libuv PR -> libuv release -> libuv upgrade in node -> node PR -> node release. You can file the node PR ahead of time but it can't land until the libuv upgrade. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
ok, got it. will work on the libuv PR, to follow the sequence. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lose
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Suggest rewording just a bit...
Moreover, save and close operations on some platforms cause inode changes
that force watch operations to become invalid and ineffective.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
One nit but otherwise LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate.
|
made the doc changes as per @jasnell . @thefourtheye - thanks, your change proposal is indirectly addressed, as the entire sentence is now modified. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@gireeshpunathil Thanks :-)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM. CI before landing: https://ci.nodejs.org/job/node-test-pull-request/6318/
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in 46edd94. Thanks! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate. Refs: #11094 Refs: #5085 PR-URL: #10085 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
|
thanks everyone! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate. Refs: #11094 Refs: #5085 PR-URL: #10085 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate. Refs: nodejs#11094 Refs: nodejs#5085 PR-URL: nodejs#10085 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate. Refs: nodejs#11094 Refs: nodejs#5085 PR-URL: nodejs#10085 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
|
should this land in v6 or v4? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I guess no, as this PR depends on aix: re-enable fs watch facility from libuv 1.11.0, which v6 and v4 haven't consumed. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate. Refs: #11094 Refs: #5085 PR-URL: #10085 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate. Refs: #11094 Refs: #5085 PR-URL: #10085 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate. Refs: #11094 Refs: #5085 PR-URL: #10085 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
On AIX, watch feature depends on AHAFS based Event infrastructure. While in principle the watch use case is same across platforms, there are subtle differences in the way AIX deals with this, with few behavioral changes (external). This commit addresses an assertion failure on folder watch, enabling the AIX code for watch feature which was masked under a macro, open up relevant test cases, skip tests which comes under the AIX limitation, and make the document changes as appropriate. Refs: nodejs/node#11094 Refs: nodejs/node#5085 PR-URL: nodejs/node#10085 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.