Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: libuv upgrade for v4.x #7205

Closed
wants to merge 6 commits into from
Closed

Conversation

saghul
Copy link
Member

@saghul saghul commented Jun 7, 2016

As discussed in: #6806

I cherry-picked all patches from master and fixed up the minor conflicts. Let me know how we want to handle the patch metadata or if it's OK like this.

/cc @nodejs/collaborators

@nodejs-github-bot nodejs-github-bot added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Jun 7, 2016
@jasnell
Copy link
Member

jasnell commented Jun 7, 2016

Definitely need to get some CI and CITGM going on this. Also, want to make sure there's a solid set of release candidates for the next LTS release just to verify this.

@saghul
Copy link
Member Author

saghul commented Jun 7, 2016

Sure thing. I'm trying to get a CI run but Jenkins seems to have other plans for tonight :-S

@saghul
Copy link
Member Author

saghul commented Jun 7, 2016

@mscdex
Copy link
Contributor

mscdex commented Jun 7, 2016

@saghul
Copy link
Member Author

saghul commented Jun 7, 2016

Thanks Brian!

@saghul
Copy link
Member Author

saghul commented Jun 8, 2016

CI is green (except a Windows 10 machine where Jenkins crapped out) and CITGM is also green.

@saghul
Copy link
Member Author

saghul commented Jun 9, 2016

How do we proceed here? Are parches from staging automagically added to RC build?

@rvagg
Copy link
Member

rvagg commented Jun 13, 2016

all in good time @saghul, mostly it's @thealphanerd who manages the v4.x-staging branch, I believe we have a couple of releases for v4.x queued up already and I for one would like this deferred until after those.

@saghul
Copy link
Member Author

saghul commented Jun 13, 2016

No problemo, I was just wondering what was the path forward. Holler if you need me!

@MylesBorins
Copy link
Contributor

MylesBorins commented Jun 13, 2016

@saghul I'm going to revisit this after the security release on Thursday. I think @nodejs/lts may have to revisit the schedule of what is planned.

edit: to clarify I am not talking about this specific fix being scheduled... but more regarding when the next patch / minor release will come, and which release we want to include this in

@MylesBorins MylesBorins force-pushed the v4.x-staging branch 2 times, most recently from 6b010a9 to 17a41c6 Compare June 24, 2016 00:29
@saghul saghul mentioned this pull request Jun 27, 2016
@MylesBorins
Copy link
Contributor

@saghul can you please rebase this against v4.x-staging?

@nodejs/ctc should we put this in staging? There was discussion in @nodejs/lts about asking @isaacs to take a look, has that discussion gone anywhere?

@saghul saghul force-pushed the upv-upgrade-v4 branch 2 times, most recently from 87094c3 to 169e331 Compare July 1, 2016 07:59
@saghul
Copy link
Member Author

saghul commented Jul 1, 2016

@thealphanerd updated.

@saghul
Copy link
Member Author

saghul commented Jul 10, 2016

Ping

@MylesBorins
Copy link
Contributor

Hey @saghul would you be able to rebase this against v4.x-staging and i'll get this landed tomorrow.

saghul and others added 5 commits July 11, 2016 10:17
Fixes: nodejs#5737
Fixes: nodejs#4643
Fixes: nodejs#4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: nodejs#3594
PR-URL: nodejs#5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Fixes: nodejs#4002
Fixes: nodejs#5384
Fixes: nodejs#6563
Refs: nodejs#2680 (comment)
PR-URL: nodejs#6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
On OSX it's possible that the fd is replaced, so use the proper libuv
API to get the correct fd.

PR-URL: nodejs#6753
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs#6838
PR-URL: nodejs#6908
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
OS X has a tiny 1kb hard-coded buffer size for stdout / stderr to
TTYs (terminals). Output larger than that causes chunking, which ends
up having some (very small but existent) delay past the first chunk.
That causes two problems:

1. When output is written to stdout and stderr at similar times, the
two can become mixed together (interleaved). This is especially
problematic when using control characters, such as \r. With
interleaving, chunked output will often have lines or characters erased
unintentionally, or in the wrong spots, leading to broken output.
CLI apps often extensively use such characters for things such as
progress bars.

2. Output can be lost if the process is exited before chunked writes
are finished flushing. This usually happens in applications that use
`process.exit()`, which isn't infrequent.

See nodejs#6980 for more info.

This became an issue as result of the Libuv 1.9.0 upgrade. A fix to
an unrelated issue broke a hack previously required for the OS X
implementation. This resulted in an unexpected behavior change in node.
The 1.9.0 upgrade was done in c3cec1e,
which was included in v6.0.0.
Full details of the Libuv issue that induced this are at
nodejs#6456 (comment)

Refs: nodejs#1771
Refs: nodejs#6456
Refs: nodejs#6773
Refs: nodejs#6816
PR-URL: nodejs#6895
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Many thanks to thefourtheye and addaleax who helped make the python
bits of this possible.

See nodejs#6980 for more info regarding
the related TTY issues.

Refs: nodejs#6456
Refs: nodejs#6773
Refs: nodejs#6816
PR-URL: nodejs#6895
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@saghul
Copy link
Member Author

saghul commented Jul 11, 2016

Rebased.

@MylesBorins
Copy link
Contributor

@MylesBorins
Copy link
Contributor

@MylesBorins
Copy link
Contributor

landed in f5756d9...a3208bd

It is worth mentioning one more time that the LTS working group decided to land these changes an include them in the v4.5.0-rc. The plan is to have the rc extensively tested, and if these changes prove to cause regressions we will be backing them out. I am fairly positive that these changes will remain, but just want to be explicitly clear about the WG decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants