Skip to content

deps: upgrade libuv to 1.9.1 - #6796

Merged
saghul merged 1 commit into
nodejs:masterfrom
saghul:uv-upgrade
May 17, 2016
Merged

deps: upgrade libuv to 1.9.1#6796
saghul merged 1 commit into
nodejs:masterfrom
saghul:uv-upgrade

Conversation

@saghul

@saghul saghul commented May 16, 2016

Copy link
Copy Markdown
Member
Checklist
  • the commit message follows commit guidelines
Affected core subsystem(s)

deps, libuv

Description of change

Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)

@nodejs-github-bot nodejs-github-bot added the libuv Issues and PRs related to the libuv dependency or the uv binding. label May 16, 2016
@saghul

saghul commented May 16, 2016

Copy link
Copy Markdown
Member Author

/cc @nodejs/collaborators

@MylesBorins

MylesBorins commented May 16, 2016

Copy link
Copy Markdown
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/2670/
citgm: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/271/

MylesBorins
MylesBorins reviewed May 16, 2016
View reviewed changes
Comment thread deps/uv/src/unix/fs.c Outdated

@MylesBorins MylesBorins May 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is worth noting that this change will make Node unable to compile on platforms that do not have PATH_MAX defined

/cc @nodejs/security @bnoordhuis

@saghul saghul May 16, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we run into such a platform we'll send them a cake as an apology. 😎

@MylesBorins MylesBorins May 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, I think this is a necessary change, although I know @bnoordhuis had mixed feelings. Mostly wanted to make sure it was clear from the beginning so there were no surprises
👼

@jasnell jasnell May 16, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The cake shouldn't exceed a specific maximum size.

@mscdex mscdex May 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The cake isn't a lie?

@jasnell jasnell May 17, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the specific maximum cake size will vary depending on whether you're running OSX or Linux. And it may or may not be fully covered in frosting by the time it's delivered. Windows users don't get cake.

@mscdex mscdex May 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There may be no frosting at all, depending on whether you called process.exit() too soon.

@jasnell jasnell May 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The cake probably won't arrive on time if you're using raspberry pi.

@BarryThePenguin BarryThePenguin mentioned this pull request May 16, 2016
Closed
@saghul

saghul commented May 17, 2016

Copy link
Copy Markdown
Member Author

The CI is 💚 except for some seemingly unrelated EAGAIN failures on one SmartOS machine and FreeBSD.

I'm not sure how to interpret the citgm results, they all seem to fail because yards couldn't be installed?

@jasnell

jasnell commented May 17, 2016

Copy link
Copy Markdown
Member

The yargs failure is hopefully just a hiccup. Everything else appears to be fine.

@eljefedelrodeodeljefe

eljefedelrodeodeljefe commented May 17, 2016

Copy link
Copy Markdown
Contributor

Couldn't find the entry, but: yargs were the first ones to move because of #6456

@MylesBorins

MylesBorins commented May 17, 2016

Copy link
Copy Markdown
Contributor

yargs failure is due to v4.7.1 tag not being published to github.

@cjihrig

cjihrig commented May 17, 2016

Copy link
Copy Markdown
Contributor

Rubber stamp LGTM

@jasnell

jasnell commented May 17, 2016

Copy link
Copy Markdown
Member

LGTM

2 similar comments
@MylesBorins

MylesBorins commented May 17, 2016

Copy link
Copy Markdown
Contributor

LGTM

@jbergstroem

jbergstroem commented May 17, 2016

Copy link
Copy Markdown
Member

LGTM

@saghul
deps: upgrade libuv to 1.9.1
b78a704 
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>
@saghul

saghul commented May 17, 2016

Copy link
Copy Markdown
Member Author

Landed! 🎉