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: upgrade to libuv 1.20.2 (formerly 1.20.1) #20129

Closed
wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 18, 2018

Fixes: #20112
Fixes: #19903

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Apr 18, 2018
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

rubber stamp LGTM
We should fast track to get this in 10.x asap

@MylesBorins MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 18, 2018
@jasnell jasnell added this to the 10.0.0 milestone Apr 18, 2018
@vsemozhetbyt
Copy link
Contributor

@Trott
Copy link
Member

Trott commented Apr 18, 2018

Not sure why macOS CI failed, but here's a re-run: https://ci.nodejs.org/job/node-test-commit-osx/17931/

AIX re-run: https://ci.nodejs.org/job/node-test-commit-aix/14325/

@richardlau
Copy link
Member

richardlau commented Apr 18, 2018

AIX compilation failure is in new code introduced by this PR, but the libuv CI for the PR that introduced it passed on AIX, libuv/libuv#1795.

../deps/uv/src/unix/thread.c:542:28: error: size of array 'static_assert_failed' is negative
 STATIC_ASSERT(sizeof(uv_sem_t) >= sizeof(uv_semaphore_t*));
                            ^~~~~~~~~~~~~~~~~~~~

@addaleax
Copy link
Member

I guess we could just move that STATIC_ASSERT into the #ifdef __GLIBC__ part of the new code … it would mask the fact that the uv__custom_sem_* methods don’t work if the condition isn’t met, but at least right now they wouldn’t be used then anyway.

I would expect for Node.js CI and libuv CI to behave the same too, though.

@richardlau
Copy link
Member

I would expect for Node.js CI and libuv CI to behave the same too, though.

Perhaps a different level of compiler is being used. There are lines like this for AIX, e.g., https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix61-ppc64/14325/console

ccache /home/iojs/gcc-6.3.0-1/opt/freeware/bin/gcc ...

which would indicate gcc 6.3.0 bring used -- @mhdawson is this expected? The most recent updates in nodejs/build#925 suggest the upgrade to gcc 6.3.0 is still in progress.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2018

We really need to try to get this landed today before I do the first RC build. I will be kicking off that build this afternoon between 1 and 2 pm pacific.

@richardlau
Copy link
Member

richardlau commented Apr 19, 2018

I think I've worked out why the libuv AIX CI passed and the Node.js AIX CI is failing -- I think libuv is building 32-bit code while Node.js is building 64-bit. The following is a reduced test showing the difference (I believe uv_sem_t ends up as a sem_t from eyeballing the code):

-bash-4.4$ cat test.c
#include <stdio.h>
#include <semaphore.h>

int main(int argc, void* argv)
{
  printf("sizeof sem_t %i\n", sizeof(sem_t));
  printf("sizeof void* %i\n", sizeof(void*));
  printf("static assert %i\n", 1-2*!(sizeof(sem_t) >= sizeof(void*)));
  return 0;
}
-bash-4.4$ gcc -o test test.c # 32-bit
-bash-4.4$ ./test
sizeof sem_t 4
sizeof void* 4
static assert 1
-bash-4.4$ gcc -o test -maix64 test.c # 64-bit
-bash-4.4$ ./test
sizeof sem_t 4
sizeof void* 8
static assert -1
-bash-4.4$

This seems to suggest that the following comment won't work on 64-bit AIX as the uv_sem_t type isn't large enough to hold a pointer:

/* To preserve ABI compatibility, we treat the uv_sem_t as storage for
 * a pointer to the actual struct we're using underneath. */

cc @addaleax

@mhdawson
Copy link
Member

We just moved up the compiler levels for 10.x (not just for AIX) so it is expected that mater is using a newer compiler. We'll have to see if the requirements for libuv have been moved up as well and we should use the newer compiler as well. That may or may not be the case will need to get input from libuv maintainers, but I think that should be in a different thread.

@mhdawson
Copy link
Member

@gireeshpunathil can you take a look as well if this is not resolved by the time you get in.

@richardlau
Copy link
Member

@mhdawson I believe the compilers being different was a red herring and the main issue is that libuv doesn't actually appear to have support for compiling 64-bit binaries on AIX (I can't find any -maix64 flags in the libuv repository) and one of the new libuv commits doesn't compile on 64-bit AIX.

@mhdawson
Copy link
Member

@richardlau ok, so we should open an issue to have the job in the libuv CI to be 64 bit instead of 32 bit. Can you do that. There still needs to be a solution to the compile failure though.

@richardlau
Copy link
Member

@mhdawson I've raised nodejs/build#1245 but it needs libuv/libuv#1807 to land first so that libuv can pass through the right compile and link flags.

@addaleax
Copy link
Member

I mean, leaving all the CI stuff aside, is there anything wrong with the suggestion from #20129 (comment)? If yes, then we can still just disable the STATIC_ASSERT completely (and temporarily) as a stop-gap measure.

@richardlau
Copy link
Member

I mean, leaving all the CI stuff aside, is there anything wrong with the suggestion from #20129 (comment)?

Sorry about being distracted with the CI. The suggestion looks okay to me.

@richardlau
Copy link
Member

Raised libuv/libuv#1808 as per #20129 (comment).

@jasnell
Copy link
Member

jasnell commented Apr 20, 2018

Is this on track to land by monday afternoon?

cjihrig pushed a commit to libuv/libuv that referenced this pull request Apr 22, 2018
Pass -Dtarget_arch=ppc64 to gyp_uv.py to activate.

Refs: nodejs/node#20129
Refs: #1795
PR-URL: #1807
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
cjihrig pushed a commit to libuv/libuv that referenced this pull request Apr 22, 2018
On 64-bit AIX `sizeof(uv_sem_t)` is 4 bytes which is not large
enough to store a pointer. AIX doesn't use glibc so the work around
introduced by #1795 doesn't
apply, so guard the STATIC_ASSERT so that it is only used when the
custom semaphore implementation is used.

Refs: nodejs/node#20129
Refs: #1795
PR-URL: #1808
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 22, 2018

Landed the relevant PRs in libuv. Here is a CI run of current Node master and libuv 1.x. macOS hasn't finished yet, but so far everything else is green. I can cut a 1.20.2 release to replace this PR before Monday afternoon (cc: @bnoordhuis @santigimeno).

UPDATE: macOS finished. It doesn't look like there were failures, but test/parallel/test-child-process-fork-net2.js left a child process behind. This appears unrelated, per #20185 (comment) from a couple hours ago.

@cjihrig cjihrig changed the title deps: upgrade to libuv 1.20.1 deps: upgrade to libuv 1.20.2 (formerly 1.20.1) Apr 22, 2018
@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 22, 2018

This has been updated to libuv 1.20.2. CI: https://ci.nodejs.org/job/node-test-pull-request/14426/

jasnell pushed a commit that referenced this pull request Apr 22, 2018
PR-URL: #20129
Fixes: #20112
Fixes: #19903
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 22, 2018

CI is green except for #18435. This should be good to go.

@jasnell
Copy link
Member

jasnell commented Apr 22, 2018

Landed in 63d991b

@jasnell jasnell closed this Apr 22, 2018
@cjihrig cjihrig deleted the libuv branch April 22, 2018 19:19
jasnell pushed a commit that referenced this pull request Apr 23, 2018
PR-URL: #20129
Fixes: #20112
Fixes: #19903
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this pull request Apr 25, 2018
The libuv 1.20.2 update in Node 10 aligned the Windows behavior
of os.uptime() with that of other operating systems. The return
value no longer contains a fraction component.

Refs: nodejs#20129
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 26, 2018
The libuv 1.20.2 update in Node 10 aligned the Windows behavior
of os.uptime() with that of other operating systems. The return
value no longer contains a fraction component.

Refs: nodejs#20129

PR-URL: nodejs#20308
Refs: nodejs#20129
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
The libuv 1.20.2 update in Node 10 aligned the Windows behavior
of os.uptime() with that of other operating systems. The return
value no longer contains a fraction component.

Refs: #20129

PR-URL: #20308
Refs: #20129
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
PR-URL: #20129
Fixes: #20112
Fixes: #19903
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
The libuv 1.20.2 update in Node 10 aligned the Windows behavior
of os.uptime() with that of other operating systems. The return
value no longer contains a fraction component.

Refs: #20129

PR-URL: #20308
Refs: #20129
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Nov 5, 2018
PR-URL: nodejs#20129
Fixes: nodejs#20112
Fixes: nodejs#19903
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2018
Backport-PR-URL: #24103
PR-URL: #20129
Fixes: #20112
Fixes: #19903
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Nov 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. 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.

Node 10, npm gives an EPERM on every operation CI failures with no output/stack trace
10 participants