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

please update the readme to avoid confusion #6

Closed
Fishrock123 opened this issue Jun 12, 2017 · 4 comments
Closed

please update the readme to avoid confusion #6

Fishrock123 opened this issue Jun 12, 2017 · 4 comments

Comments

@Fishrock123
Copy link

Someone I know was somewhat confused if this was the main node repo or not, we should probably either patch the readme, or be much more clear in the repo name + description.
@bnb
Copy link

bnb commented Jul 24, 2017

+1 - is it possible to get an update on this?

@tniessen
Copy link
Member

I would suggest to update the description - e.g. Node.js mirror on ... instead of Node.js on ....

fhinkel added a commit to fhinkel/node-v8 that referenced this issue Jul 25, 2017
@fhinkel
Update Node.js readme to avoid confusion
edc1b7b 
We want to make sure nobody confuses this repo with the main repo.

Ref: nodejs#6
fhinkel added a commit to fhinkel/node-v8 that referenced this issue Jul 25, 2017
@fhinkel
Update Node.js readme to avoid confusion
d3b6124 
We want to make sure nobody confuses this repo with the main repo.

Ref: nodejs#6
fhinkel added a commit to fhinkel/node-v8 that referenced this issue Jul 25, 2017
@fhinkel
Update Node.js readme to avoid confusion
7a6154c 
We want to make sure nobody confuses this repo with the main repo.

Ref: nodejs#6
@fhinkel fhinkel mentioned this issue Jul 25, 2017
Closed
fhinkel added a commit to fhinkel/node-v8 that referenced this issue Aug 1, 2017
@fhinkel
Update Node.js readme to avoid confusion
b087f9c 
We want to make sure nobody confuses this repo with the main repo.

Ref: nodejs#6
targos pushed a commit that referenced this issue Aug 3, 2017
@fhinkel @targos
Update Node.js readme to avoid confusion
1a93216 
We want to make sure nobody confuses this repo with the main repo.

Ref: #6
@targos
Copy link
Member

targos commented Aug 3, 2017

Updated in #13

To change the contents of the new README, one can open a PR against the readme branch.

@targos targos closed this as completed Aug 3, 2017
@targos
Copy link
Member

targos commented Aug 3, 2017

I also changed the description to "Experimental Node.js mirror on V8 Tip-of-Tree"

nodejs-ci pushed a commit that referenced this issue Oct 27, 2017
@danbev
test: allow tests to pass without internet
d653f03 
Currently when running the test without an internet connection there are
two JavaScript test failures and one cctest. The cctest only fails on
Mac as far as I know. (I've only tested using Mac and Linux thus far).

This commit moves the two JavaScript tests to test/internet.

The details for test_inspector_socket_server.cc:

[ RUN      ] InspectorSocketServerTest.FailsToBindToNodejsHost
make[1]: *** [cctest] Segmentation fault: 11
make: *** [test] Error 2

lldb output:

[ RUN      ] InspectorSocketServerTest.FailsToBindToNodejsHost
Process 63058 stopped
* thread #1: tid = 0x7b175, 0x00007fff96d04384
* libsystem_info.dylib`_gai_simple + 87, queue =
* 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1,
* address=0x0)
    frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87
libsystem_info.dylib`_gai_simple:
->  0x7fff96d04384 <+87>: movw   (%rdx), %ax
    0x7fff96d04387 <+90>: movw   %ax, -0x2a(%rbp)
    0x7fff96d0438b <+94>: movq   %r13, -0x38(%rbp)
    0x7fff96d0438f <+98>: movq   0x18(%rbp), %rcx

(lldb) bt
* thread #1: tid = 0x7b175, 0x00007fff96d04384
* libsystem_info.dylib`_gai_simple + 87, queue =
* 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1,
* address=0x0)
  * frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87
    frame #1: 0x00007fff96cfe98b libsystem_info.dylib`search_addrinfo +
179
    frame #2: 0x00007fff96cfafef libsystem_info.dylib`si_addrinfo + 2255
    frame #3: 0x00007fff96cfa67b libsystem_info.dylib`getaddrinfo + 179
    frame #4: 0x00000001017d8888
cctest`uv__getaddrinfo_work(w=0x00007fff5fbfe210) + 72 at
getaddrinfo.c:102
    frame #5: 0x00000001017d880e
cctest`uv_getaddrinfo(loop=0x000000010287cb80, req=0x00007fff5fbfe1c8,
cb=0x0000000000000000, hostname="nodejs.org", service="0",
hints=0x00007fff5fbfe268) + 734 at getaddrinfo.c:192
    frame #6: 0x000000010171f781
cctest`node::inspector::InspectorSocketServer::Start(this=0x00007fff5fbfe658)
+ 801 at inspector_socket_server.cc:398
    frame #7: 0x00000001016ed590
cctest`InspectorSocketServerTest_FailsToBindToNodejsHost_Test::TestBody(this=0x0000000105001fd0)
+ 288 at test_inspector_socket_server.cc:593

I'm not sure about the exact cause for this but when using a standalone
c program to simulate this it seems like when the ai_flags
`AI_NUMERICSERV` is set, which is done in inspector_socket_server.cc
line 394, the servname (the port in the FailsToBindToNodejsHost test) is
expected to be a numeric port string to avoid looking it up in
/etc/services. When the port is 0 as is it was before this commit the
segment fault occurs but not if it is non-zero.

PR-URL: nodejs/node#16255
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@guybedford guybedford mentioned this issue Jun 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bnb @Fishrock123 @targos @tniessen