Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

deps: update libuv to 0.10.36 #9274

Closed
wants to merge 1 commit into from
Closed

deps: update libuv to 0.10.36 #9274

wants to merge 1 commit into from

Conversation

saghul
Copy link
Member

@saghul saghul commented Feb 24, 2015

@saghul
Copy link
Member Author

saghul commented Feb 24, 2015

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 saghul changed the title deps: update libuv to 0.10.34 deps: update libuv to 0.10.35 Feb 24, 2015
@misterdjules
Copy link

@saghul Thank you!

As far as I know, simple/test-http-many-keep-alive-connections failing is expected. However, test-child-process-fork-net2.js and test-signal-unregister.js failing is new to me.

@saghul saghul changed the title deps: update libuv to 0.10.35 deps: update libuv to 0.10.36 Feb 26, 2015
@saghul
Copy link
Member Author

saghul commented Feb 26, 2015

Force pushed updating to 0.10.36. There was a problem with 0.10.35 which was caught by iojs.

@saghul
Copy link
Member Author

saghul commented Feb 27, 2015

@misterdjules can you please re-run the CI? Locally (Linux) I only get the expected failure:

git|uv_01034)saghul@alpha:~/src/node
$ make test-simple 
make -C out BUILDTYPE=Release V=1
make[1]: Entering directory '/home/saghul/src/node/out'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/saghul/src/node/out'
ln -fs out/Release/node node
/usr/bin/python tools/test.py simple
=== release test-http-many-keep-alive-connections ===                          
Path: simple/test-http-many-keep-alive-connections
Command: out/Release/node /home/saghul/src/node/test/simple/test-http-many-keep-alive-connections.js
--- TIMEOUT ---
[02:34|% 100|+ 621|-   1]: Done                                                
Makefile:114: recipe for target 'test-simple' failed
make: *** [test-simple] Error 1

@misterdjules
Copy link

@misterdjules
Copy link

@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?

@tjfontaine
Copy link

I believe (unfortunately) that tls-session-cache is an expected failure currently, but deserves a second look, and can probably be verified by looking at the test results for v0.10.36-release.

net-error-twice seems like a behavior change on the order on which we expect events to happen, and does seem like a regression in behavior which should be further investigated, though the test itself does seem like it is slightly non-deterministic.

@misterdjules
Copy link

@saghul @joyent/node-coreteam test-tls-session-cache.js failing on Linux has happened before, and it seems it should be put in the same category of "spurious failures".

I went back in the history of the nodejs-v0.10 Jenkins job, and haven't found yet any occurence of test-net-error-twice.js failing on SmartOS. The only "expected spurious failure" I found on SmartOS so far is test-child-process-fork-net2.js.

@trevnorris
Copy link

So is there still a problem with test-net-error-twice?

@misterdjules
Copy link

@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.

@saghul
Copy link
Member Author

saghul commented Mar 3, 2015

FWIW, I don't see how that test failing can be a libuv issue :-S

@trevnorris
Copy link

@saghul Same here. Don't see how the two are related.

@misterdjules
Copy link

@saghul @trevnorris Looking at it now.

@misterdjules
Copy link

@saghul @trevnorris Indeed, test-net-error-twice.js' failure is not due to this libuv upgrade. I'll create a separate issue for this but it shouldn't prevent landing this change.

@misterdjules
Copy link

Just FYI, created #9325 about test-net-error-twice.js failing.

@misterdjules
Copy link

@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; \

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*).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 it looks ok.

@misterdjules
Copy link

@saghul @trevnorris @tjfontaine See my comments inline, but otherwise LGTM.

@saghul
Copy link
Member Author

saghul commented Mar 5, 2015

No, it doesn't in this case because its a union between the type and a
pointer to it.
On Mar 5, 2015 9:07 PM, "Julien Gilli" notifications@github.com wrote:

@saghul https://github.com/saghul @trevnorris
https://github.com/trevnorris @tjfontaine
https://github.com/tjfontaine See my comments inline, but otherwise
LGTM.


Reply to this email directly or view it on GitHub
#9274 (comment).

@misterdjules
Copy link

@saghul Ok thanks, we agree on that then 👍

@misterdjules
Copy link

@saghul Did you see this comment too?

@saghul
Copy link
Member Author

saghul commented Mar 6, 2015

@misterdjules sorry, missed that one. Just replied.

@misterdjules
Copy link

@saghul Thank you! @joyent/node-coreteam Landing asap.

misterdjules pushed a commit to misterdjules/node that referenced this pull request Mar 6, 2015
PR: nodejs#9274
PR-URL: nodejs#9274
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
@misterdjules
Copy link

Thank you again @saghul, landed in dcff5d5!

@saghul
Copy link
Member Author

saghul commented Mar 6, 2015

Great! \o/
On Mar 6, 2015 8:44 PM, "Julien Gilli" notifications@github.com wrote:

Thank you again @saghul https://github.com/saghul, landed in dcff5d5
dcff5d5
!


Reply to this email directly or view it on GitHub
#9274 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants