-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Conversation
Thanks @saghul! @tjfontaine Tests results for Unices: http://jenkins.nodejs.org/job/node-review-unix-julien/42/ and Windows: http://jenkins.nodejs.org/job/node-review-windows-julien/34/. So far, I've seen two suspicious failures, only on SmartOS: http://jenkins.nodejs.org/job/node-review-unix-julien/42/DESTCPU=ia32,label=smartos/tapTestReport/simple.tap-30/ and http://jenkins.nodejs.org/job/node-review-unix-julien/42/DESTCPU=x64,label=smartos/tapTestReport/simple.tap-473/. |
Make that 0.10.35 :-) simple/test-http-many-keep-alive-connections seems to consistently fail on my machine, not sure about the upgrade is the culprit though. |
@saghul Thank you! As far as I know, |
Force pushed updating to 0.10.36. There was a problem with 0.10.35 which was caught by iojs. |
@misterdjules can you please re-run the CI? Locally (Linux) I only get the expected failure:
|
@saghul Building/testing here: http://jenkins.nodejs.org/job/node-review-julien/31/default/. |
@saghul Builds results for UNICEs here: http://jenkins.nodejs.org/job/node-review-unix-julien/ and for Windows here: http://jenkins.nodejs.org/job/node-review-windows-julien/. The only tests failing that I'm not sure are expected to fail are: The rest of the failures look to me like they're expected. @joyent/node-coreteam Thoughts? |
I believe (unfortunately) that
|
@saghul @joyent/node-coreteam I went back in the history of the nodejs-v0.10 Jenkins job, and haven't found yet any occurence of |
So is there still a problem with |
@trevnorris I haven't had the time to investigate, I'll put that at the top of my priority list and hopefully I can find some time to work on it soon. |
FWIW, I don't see how that test failing can be a libuv issue :-S |
@saghul Same here. Don't see how the two are related. |
@saghul @trevnorris Looking at it now. |
@saghul @trevnorris Indeed, |
Just FYI, created #9325 about |
@saghul @trevnorris I'll try to do an actual code review tomorrow (Thursday) now that we know that no regression has been introduced by this upgrade. |
AFD_POLL_INFO* afd_poll_info_ptr; \ | ||
AFD_POLL_INFO afd_poll_info; \ | ||
} afd_poll_info_2; \ | ||
AFD_POLL_INFO afd_poll_info_2; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to make sure we acknowledge that this doesn't change the ABI because sizeof(AFD_POLL_INFO) >= sizeof(AFD_POLL_INFO*).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 it looks ok.
@saghul @trevnorris @tjfontaine See my comments inline, but otherwise LGTM. |
No, it doesn't in this case because its a union between the type and a
|
@saghul Ok thanks, we agree on that then 👍 |
@saghul Did you see this comment too? |
@misterdjules sorry, missed that one. Just replied. |
@saghul Thank you! @joyent/node-coreteam Landing asap. |
PR: nodejs#9274 PR-URL: nodejs#9274 Reviewed-By: Julien Gilli <[email protected]>
Great! \o/
|
R=@tjfontaine