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

added stubs for tcgetattr and tcsetattr, minor node tty fixes #1555

Merged
merged 4 commits into from
Aug 30, 2013

Conversation

inolen
Copy link
Collaborator

@inolen inolen commented Aug 25, 2013

...

@kripken
Copy link
Member

kripken commented Aug 26, 2013

Any chance for a test here? :)

@inolen
Copy link
Collaborator Author

inolen commented Aug 26, 2013

I'll write up some tests later today, sure.

@inolen
Copy link
Collaborator Author

inolen commented Aug 28, 2013

Writing the tests for the tty ended up being more work than I'd planned. I decided to make test_stdin work for node. It wasn't that node's stdin read support was fundamentally broken, it's just entirely asynchronous, and the test was updated to treat it as such.

A few minor fixes had to be made to the tty library in order to support reading from text files that were piped to stdin. Currently, the FS startup code doesn't distinguish if stdin is a tty device or not (it always assumes it's a tty device), so the tty library now forces process.stdin to be utf8 encoded. This makes reading from text files work, but really there should be some refactoring to the standard stream initialization such that the TTY device isn't being used when stdin represents a file. Also, there was a really nasty issue where if you read only once from stdin the process would hang in node, I left a short IRC transcript from node's streams author in there explaining the "fix."

Finally, I hit a few snags in libc calls:

  • clearerr should reset both eof and error indicators
  • fgetc was incorrectly setting the eof indicator. in cases where fread had errored with EAGAIN it was setting eof. I removed the set entirely, as there is no need for fgetc to even worry about it, fread will set the correct value in any case.

@kripken
Copy link
Member

kripken commented Aug 28, 2013

Does test_stdin still work on spidermonkey? Let's loop on spidermonkey and node to test both.

@inolen
Copy link
Collaborator Author

inolen commented Aug 28, 2013

Err, I went to try and run it in SM but got errors about setTimeout not being defined:

anthonypesch@lembp:~/projects/emscripten$ js
js> setTimeout
typein:1: ReferenceError: setTimeout is not defined

If this is true (does SM shell not implement an event loop?) I guess I can update the test to manually call main_loop once before hitting emscripten_set_main_loop to be compatible in SM.

@inolen
Copy link
Collaborator Author

inolen commented Aug 28, 2013

Updated the test to work in SM shell, let me know if that comment is incorrect.

Do you want the test runner code to manually iterate over both engines?

@kripken
Copy link
Member

kripken commented Aug 28, 2013

Yeah, just loop over [NODE_JS, SPIDERMONKEY_ENGINE].

@kripken
Copy link
Member

kripken commented Aug 28, 2013

Yes, no async stuff in sm shell.

@kripken
Copy link
Member

kripken commented Aug 29, 2013

Needs a rebase, says it can't be merged.

 - added fixes for tty get_char in the node environment
 - made test_stdin async to work in the node environment
 - clearerr should reset both eof and error indicators
 - fgetc was incorrectly setting the eof indicator. in cases where fread had errored with EAGAIN it was setting eof. I removed the set entirely, as there is no need for fgetc to even worry about it, fread will set the correct value in any case
@inolen
Copy link
Collaborator Author

inolen commented Aug 29, 2013

Rebased, test_pthread_specific was added at the exact same line messing up the auto-merge I suppose.

kripken added a commit that referenced this pull request Aug 30, 2013
added stubs for tcgetattr and tcsetattr, minor node tty fixes
@kripken kripken merged commit 77c4a7e into emscripten-core:incoming Aug 30, 2013
@kripken
Copy link
Member

kripken commented Aug 30, 2013

test_stdin does not pass for me in node, i had to disable it.

I get error exit(1) called.

@inolen
Copy link
Collaborator Author

inolen commented Aug 30, 2013

K, I'll take a look in my VM today. The code in the TTY modules feels kind of bad, but I'd like to get it resolved in node.

@inolen
Copy link
Collaborator Author

inolen commented Sep 3, 2013

While this fixed a few issues, it also created a new issue where Python is exiting with EAGAIN when piping the output during the test runner. You'll notice it when a test fails and all you see is:

IOError: [Errno 35] Resource temporarily unavailable

For whatever reason, by simply referencing process.stdin, e.g.:

process.stdin;

node does something such that reading from its stdout is returning EAGAIN instead of blocking.

Pushed a fix disabling TTY::init and TTY::quit until I can come up with a proper solution as reading the test runner is a bit more important than supporting reading text files through stdin in node:
9451cad

@inolen
Copy link
Collaborator Author

inolen commented Sep 4, 2013

Posted an issue in the node issue queue regarding this:

nodejs/node-v0.x-archive#6175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants