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.

Sign up for GitHub

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

Jump to bottom

deps: cherry-pick e7f4e9e from upstream libuv #16724

Closed
wants to merge 1 commit into from
Closed

deps: cherry-pick e7f4e9e from upstream libuv #16724

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

Original commit message:
tty, win: get SetWinEventHook pointer at startup

SetWinEventHook is not available on some Windows versions.

Fixes: https://github.com/nodejs/node/issues/16603
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

fixes: #16603

A change in libuv has broken the ability to compile node in Windows NanoServer container. Floating this patch fixes it 🎉

@bzoz @MylesBorins
deps: cherry-pick e7f4e9e from upstream libuv
eac9813 
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: nodejs#16603
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

fixes: nodejs#16603
@nodejs-github-bot nodejs-github-bot added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Nov 3, 2017
This was referenced Nov 3, 2017
Closed
Closed
@MylesBorins
Copy link
Contributor Author

I would like to see this patch land ASAP so we can land on 9.x, 8.x, 6.x and test over the weekend

@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Closed
bnoordhuis
bnoordhuis approved these changes Nov 3, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM, I guess, but I'm old and set in my ways: why float a patch when we can just upgrade libuv wholesale?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 3, 2017

@bnoordhuis I'm guessing since this will go into an LTS release almost immediately. If we release 1.16.0 and update to that, there is a chance of other regressions going directly into v6.

@bnoordhuis
Copy link
Member

That makes sense, and I'm okay with it because it's Windows.

Floating a UNIX fix as a patch however means distro builds miss out on it because they link to libuv.so, not our bundled copy.

What I mean to say is, let's not turn it into a new tradition.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 3, 2017

Good point, as usual.

What I mean to say is, let's not turn it into a new tradition.

Completely agree.

@MylesBorins
Copy link
Contributor Author

@cjihrig @bnoordhuis if y'all want to cut a 1.15.1 with this patch I'd be more than happy to do a more official upgrade. Open to whatever y'all think it best

@cjihrig
Copy link
Contributor

cjihrig commented Nov 3, 2017

In this case I think I'd rather just go with a cherry-pick. I'll likely be doing a full 1.16.0 release early next week and updating Node with that.

@MylesBorins
Copy link
Contributor Author

CI against rebased master: https://ci.nodejs.org/job/node-test-pull-request/11181/

if this is green I plan to land on master, 9.x-staging, 8.x-staging, and 6.x-staging

/cc @nodejs/tsc @nodejs/lts please lmk if you have any objects to landing quickly

@gibfahn
Copy link
Member

gibfahn commented Nov 3, 2017

+1 to landing quickly.

@MylesBorins
Copy link
Contributor Author

landed in 92f8663

@MylesBorins MylesBorins closed this Nov 4, 2017
MylesBorins pushed a commit that referenced this pull request Nov 4, 2017
@bzoz @MylesBorins
deps: cherry-pick e7f4e9e from upstream libuv
92f8663 
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: #16603
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #16724
Fixes: #16603
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 4, 2017
@bzoz @MylesBorins
deps: cherry-pick e7f4e9e from upstream libuv
ed0fbd8 
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: #16603
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #16724
Fixes: #16603
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 4, 2017
@bzoz @MylesBorins
deps: cherry-pick e7f4e9e from upstream libuv
a815e1b 
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: #16603
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #16724
Fixes: #16603
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 4, 2017
@bzoz @MylesBorins
deps: cherry-pick e7f4e9e from upstream libuv
b98fa82 
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: #16603
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #16724
Fixes: #16603
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins
Copy link
Contributor Author

landed on v9.x-staging, v8.x-staging, and v6.x-staging

@gibfahn gibfahn mentioned this pull request Nov 5, 2017
Merged
@MylesBorins MylesBorins mentioned this pull request Nov 6, 2017
Merged
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
@MylesBorins MylesBorins deleted the libuv-patch branch November 14, 2017 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

node.exe 8.7.0 and above does not work in Windows NanoServer container
7 participants
@MylesBorins @cjihrig @bnoordhuis @gibfahn @refack @nodejs-github-bot @bzoz