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

[NODERAWFS] Revert #18163 #22393

Merged

Conversation

kleisauke
Copy link
Collaborator

@kleisauke kleisauke commented Aug 17, 2024

This reverts commit c614fa5.


Here's a standalone reproducer:

Before:

$ curl -LO https://github.com/llvm/llvm-test-suite/raw/main/SingleSource/Benchmarks/Misc-C++/Large/ray.cpp
$ em++ -O3 -sEXIT_RUNTIME -sNODERAWFS ray.cpp -o ray.js
$ node ray.js > x.ppm
$ sha256sum x.ppm
acaa1db08ee30dafda03b41c77466b1435fe8ffdd4d88e5a5876e87dff126f58  x.ppm
$ vips copy x.ppm x.png
VipsImage: memory area too small --- should be 262144 bytes, you passed 79485

After:

$ em++ -O3 -sEXIT_RUNTIME -sNODERAWFS ray.cpp -o ray.js
$ node ray.js > x.ppm
$ sha256sum x.ppm
cd6fa6fa75b4ba29077f7e4e531b96f17ff0b82f3c70df526c36f748d6a38c5c  x.ppm
$ vips copy x.ppm x.png
x.png

x

@kleisauke kleisauke force-pushed the noderawfs-fix-unflushed-content branch from 167507d to 4472431 Compare August 18, 2024 13:50
@kleisauke kleisauke changed the title [NODERAWFS] Ensure stdio streams are flushed on exit [NODERAWFS] Avoid library_tty.js for stdio streams Aug 18, 2024
@sbc100
Copy link
Collaborator

sbc100 commented Aug 19, 2024

I'm inclined to perhaps revisit #18163. It seems to me that if one uses NODERAWFS then overriding stdout, stderr, stdin, print, printErr should make just not effect the program stdout/stderr/stdin since the whole point of NODERAWFS is to directly map to node file I/O.

@kleisauke kleisauke force-pushed the noderawfs-fix-unflushed-content branch from 8b5b56d to 69b7076 Compare August 19, 2024 19:52
@kleisauke
Copy link
Collaborator Author

I'm inclined to perhaps revisit #18163. It seems to me that if one uses NODERAWFS then overriding stdout, stderr, stdin, print, printErr should make just not effect the program stdout/stderr/stdin since the whole point of NODERAWFS is to directly map to node file I/O.

I agree, let's revert #18163. In retrospect, it seems that library_tty.js is very tied to the browser, as it skips null terminators and always flushes with a newline.

The latest revision of this PR reverts #18163 in a trivial way.

test/test_other.py Outdated Show resolved Hide resolved
test/test_other.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Can you update the PR title to refer to the change you are reverting?

test/test_other.py Outdated Show resolved Hide resolved
@@ -55,6 +54,12 @@ addToLibrary({
var mode = NODEFS.getMode(path);
return { path, node: { id: st.ino, mode, node_ops: NODERAWFS, path }};
},
createStandardStreams() {
FS.createStream({ nfd: 0, position: 0, path: '', flags: 0, tty: false, seekable: false }, 0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that I set tty: false here to ensure that issue #22264 remains resolved.

$ emcc ioctl_test.c -lnodefs.js -lnoderawfs.js -sEXIT_RUNTIME
$ node ./a.out.js
pc-kaw
a.out.js: /dev/stdin not a tty: Not a tty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I also set it for stdout and stderr, btw)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems reasonable for now, but we should probably add a TODO there, and we should really be setting tty based on the underlying node isatty call for each descriptor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I just noticed that isatty for stdio streams will return 0 and set errno to ENOTTY with this change, which is probably not what we want. I just pushed commit 88a5475 instead. Let's reopen #22264 after this change.

@kleisauke kleisauke changed the title [NODERAWFS] Avoid library_tty.js for stdio streams [NODERAWFS] Revert #18163 Aug 19, 2024
@kleisauke
Copy link
Collaborator Author

Can you update the PR title to refer to the change you are reverting?

Sure, updated to [NODERAWFS] Revert #18163.

@sbc100 sbc100 merged commit 659f01d into emscripten-core:main Aug 20, 2024
25 of 27 checks passed
@kleisauke kleisauke deleted the noderawfs-fix-unflushed-content branch August 20, 2024 17:34
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