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

gh-84461: Improve WebAssembly in-browser demo #91879

Merged
merged 10 commits into from
Jul 1, 2022
Merged

gh-84461: Improve WebAssembly in-browser demo #91879

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
38f7ed9
Buffer standard input line-by-line
treyhunner Apr 24, 2022
c5365f9
Add non-root .editorconfig for JS & HTML indent
treyhunner Apr 24, 2022
292d32c
Add support for clearing REPL with CTRL+L
treyhunner Apr 24, 2022
37170bb
Support unicode in stdout and stderr
treyhunner Apr 24, 2022
c99b827
Remove \r\n normalization
treyhunner Apr 24, 2022
738dad4
Note that local .editorconfig file extends root
treyhunner Apr 24, 2022
3d9048c
Only normalize lone \r characters (convert to \n)
treyhunner Apr 26, 2022
16d53b0
Skip non-printable characters in buffered input
treyhunner May 9, 2022
2c99fdb
Fix Safari bug (regex lookbehind not supported)
treyhunner May 27, 2022
be8a114
Merge branch 'main' into improve-wasm-demo
tiran Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Tools/wasm/.editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
root = false # This extends the root .editorconfig

[*.{html,js}]
trim_trailing_whitespace = true
insert_final_newline = true
indent_style = space
indent_size = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure that file is required

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right that it's not strictly required, but CPython already has a root .editorconfig file which informs text editors of the indentation and whitespace style to use. The root file doesn't have rules for HTML and JS files, which I noticed because my editor had a default setup that was inserting 2 spaces when I hit tab in these files.

I'm not sure whether the other *.js and *.html files in this repository use 4 spaces consistently, so I made a local .editorconfig file instead of adding to the root one.

109 changes: 90 additions & 19 deletions Tools/wasm/python.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
class WasmTerminal {

constructor() {
this.inputBuffer = new BufferQueue();
this.input = ''
this.resolveInput = null
this.activeInput = false
Expand All @@ -123,28 +124,47 @@
this.xterm.open(container);
}

handleReadComplete(lastChar) {
this.resolveInput(this.input + lastChar)
this.activeInput = false
}

handleTermData = (data) => {
if (!this.activeInput) {
return
}
const ord = data.charCodeAt(0);
let ofs;
data = data.replace(/\r(?!\n)/g, "\n") // Convert lone CRs to LF

// Handle pasted data
if (data.length > 1 && data.includes("\n")) {
let alreadyWrittenChars = 0;
// If line already had data on it, merge pasted data with it
if (this.input != '') {
this.inputBuffer.addData(this.input);
alreadyWrittenChars = this.input.length;
this.input = '';
}
this.inputBuffer.addData(data);
// If input is active, write the first line
if (this.activeInput) {
let line = this.inputBuffer.nextLine();
this.writeLine(line.slice(alreadyWrittenChars));
this.resolveInput(line);
this.activeInput = false;
}
// When input isn't active, add to line buffer
} else if (!this.activeInput) {
// Skip non-printable characters
if (!(ord === 0x1b || ord == 0x7f || ord < 32)) {
this.inputBuffer.addData(data);
}
// TODO: Handle ANSI escape sequences
if (ord === 0x1b) {
} else if (ord === 0x1b) {
// Handle special characters
} else if (ord < 32 || ord === 0x7f) {
switch (data) {
case "\r": // ENTER
case "\x0c": // CTRL+L
this.clear();
break;
case "\n": // ENTER
case "\x0a": // CTRL+J
case "\x0d": // CTRL+M
this.xterm.write('\r\n');
this.handleReadComplete('\n');
this.resolveInput(this.input + this.writeLine('\n'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why you are writing \n here if a) you normalize \r and \n to \r\n above, and b) you writeline so you write a \r\n after?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe flush on '\n' 0xa only instead of '\r' (0xd) ? or better begin to handle canonical and raw terminal differently as they should. LF should not be treated as EOL.( ncurses and ncursesw will be very soon available on wasm ).

Copy link
Contributor

@pmp-p pmp-p Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

i also noticed that isatty() is returning 0 for fd 0 / 1 / 2 and it should not
maybe try

    SYSCALLS.getStreamFromFD(0).tty = true;
    SYSCALLS.getStreamFromFD(1).tty = true;
    SYSCALLS.getStreamFromFD(2).tty = true;

at module init before trying to fix something that may not be broken :

>>> sys._emscripten_info
sys._emscripten_info(emscripten_version=(3, 1, 10), runtime='Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.4896.127 Safari/537.36', pthreads=False, shared_memory=False)
>>> 
>>> 
>>> print("                   123\r456",end="")
456>>>             123

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you explain why you are writing \n here if a) you normalize \r and \n to \r\n above, and b) you writeline so you write a \r\n after?

That inconsistency was unintentional. Python seems to have translated \r\n to simply \n in the strings and I hadn't noticed my mistake. In fact that case condition could never be reached due to \r always being converted to \r\n (due to the data.length > 1 check above).

I just pushed a commit that normalizes lone \r to \n but keeps \r\n intact (and allows this case statement to actually run).

The writeLine doesn't actually write to Python. It writes to xterm only. The xterm session needs \r\n (it gets unhappy otherwise: #91879 (comment)) but writeLine returns its input (\n in this case) which is then written to standard input. So this.resolveInput ends up being passed simply this.input + '\n'.

this.input = '';
this.activeInput = false;
break;
case "\x7F": // BACKSPACE
case "\x08": // CTRL+H
Expand All @@ -157,6 +177,12 @@
}
}

writeLine(line) {
this.xterm.write(line.slice(0, -1))
this.xterm.write('\r\n');
return line;
}

handleCursorInsert(data) {
this.input += data;
this.xterm.write(data)
Expand All @@ -176,9 +202,19 @@
this.activeInput = true
// Hack to allow stdout/stderr to finish before we figure out where input starts
setTimeout(() => {this.inputStartCursor = this.xterm.buffer.active.cursorX}, 1)
// If line buffer has a line ready, send it immediately
if (this.inputBuffer.hasLineReady()) {
return new Promise((resolve, reject) => {
resolve(this.writeLine(this.inputBuffer.nextLine()));
this.activeInput = false;
})
// If line buffer has an incomplete line, use it for the active line
} else if (this.inputBuffer.lastLineIsIncomplete()) {
// Hack to ensure cursor input start doesn't end up after user input
setTimeout(() => {this.handleCursorInsert(this.inputBuffer.nextLine())}, 1);
}
return new Promise((resolve, reject) => {
this.resolveInput = (value) => {
this.input = ''
resolve(value)
}
})
Expand All @@ -188,9 +224,44 @@
this.xterm.clear();
}

print(message) {
const normInput = message.replace(/[\r\n]+/g, "\n").replace(/\n/g, "\r\n");
this.xterm.write(normInput);
print(charCode) {
let array = [charCode];
if (charCode == 10) {
array = [13, 10]; // Replace \n with \r\n
Copy link
Contributor

Choose a reason for hiding this comment

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

python should send \r\n not \n, line should be cooked for a terminal on stdout. add a workaround or fix tty detection in emscripten ?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I remove that \n -> \r\n replacement code xterm.js looks like this:

image

When I did a console.log for \r and \n, I saw \n printed out but \r never seemed to come out of Python:

        if (charCode == 10) {
            console.log('Saw LF');
            array = [13, 10];  // Replace \n with \r\n
        } else if (charCode == 13) {
            console.log('Saw CR');
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a problem with emscripten Module I/O registration, i'm not sure but it may have to do with something like TTY.register() could not find much apart from a vague mention that it does not have any doc see emscripten-core/emscripten#6861

Copy link
Contributor

@katharosada katharosada Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Emscripten's API is unfortunately not really a TTY and seems to be designed to work with a browser console.log for debugging more than anything else. For example, it doesn't flush without a newline character. The web REPL has no choice but to work around it. 😐

To make the Python REPL actually work nicely, you either have to do a lot more work on the JS/UI side to add behaviour (like this PR does) or add better TTY support to emscripten itself and eventually use readline (which would be awesome).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also be very interested in getting readline to work. I think with threading enabled it would be easy. With Python in a webworker it is probably possible, but it would probably require rewriting readline_until_enter_or_signal and significantly patching the I/O in gnu readline itself. So maybe not completely impossible but a significant amount of work.

}
this.xterm.write(new Uint8Array(array));
}
}

class BufferQueue {
constructor(xterm) {
this.buffer = []
}

isEmpty() {
return this.buffer.length == 0
}

lastLineIsIncomplete() {
return !this.isEmpty() && !this.buffer[this.buffer.length-1].endsWith("\n")
}

hasLineReady() {
return !this.isEmpty() && this.buffer[0].endsWith("\n")
}

addData(data) {
let lines = data.split(/(?<=\n)/g)
if (this.lastLineIsIncomplete()) {
this.buffer[this.buffer.length-1] += lines.shift()
}
for (let line of lines) {
this.buffer.push(line)
}
}

nextLine() {
return this.buffer.shift()
}
}

Expand All @@ -202,8 +273,8 @@
terminal.open(document.getElementById('terminal'))

const stdio = {
stdout: (s) => { terminal.print(s) },
stderr: (s) => { terminal.print(s) },
stdout: (charCode) => { terminal.print(charCode) },
stderr: (charCode) => { terminal.print(charCode) },
stdin: async () => {
return await terminal.prompt()
}
Expand Down
8 changes: 2 additions & 6 deletions Tools/wasm/python.worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,11 @@ class StdinBuffer {
}
}

const stdoutBufSize = 128;
const stdoutBuf = new Int32Array()
let index = 0;

const stdout = (charCode) => {
if (charCode) {
postMessage({
type: 'stdout',
stdout: String.fromCharCode(charCode),
stdout: charCode,
})
} else {
console.log(typeof charCode, charCode)
Expand All @@ -54,7 +50,7 @@ const stderr = (charCode) => {
if (charCode) {
postMessage({
type: 'stderr',
stderr: String.fromCharCode(charCode),
stderr: charCode,
})
} else {
console.log(typeof charCode, charCode)
Expand Down