From feeadd5fff8917cdff7b7831be3c3226a388e0a1 Mon Sep 17 00:00:00 2001 From: Hood Chatham Date: Thu, 9 Sep 2021 11:53:47 -0700 Subject: [PATCH] FIX Patch emscripten to fix problems with dup (#1823) --- docs/project/changelog.md | 7 ++++ emsdk/patches/fix-dup.patch | 75 +++++++++++++++++++++++++++++++++++++ src/tests/test_pyodide.py | 36 ++++++++++++++++++ 3 files changed, 118 insertions(+) create mode 100644 emsdk/patches/fix-dup.patch diff --git a/docs/project/changelog.md b/docs/project/changelog.md index 3e3411fc9..88fa2f194 100644 --- a/docs/project/changelog.md +++ b/docs/project/changelog.md @@ -69,6 +69,13 @@ substitutions: - {{Fix}} pillow now correctly encodes/decodes JPEG image format. {pr}`1818` +### Micellaneous + +- {{Fix}} Patched emscripten to make the system calls to duplicate file + descriptors closer to posix-compliant. In particular, this fixes the use of + `dup` on pipes and temporary files, as needed by `pytest`. + {pr}`1823` + ## Version 0.18.0 _August 3rd, 2021_ diff --git a/emsdk/patches/fix-dup.patch b/emsdk/patches/fix-dup.patch new file mode 100644 index 000000000..33eae61e1 --- /dev/null +++ b/emsdk/patches/fix-dup.patch @@ -0,0 +1,75 @@ +From 52c0b163bad96cde2081c734680b9ec485fea908 Mon Sep 17 00:00:00 2001 +From: Hood +Date: Wed, 8 Sep 2021 17:49:15 -0700 +Subject: [PATCH] Fix dup + +--- + src/library_syscall.js | 12 ++++++------ + tests/third_party/posixtestsuite | 2 +- + 2 files changed, 7 insertions(+), 7 deletions(-) + +This should fix two problems with the `dup` system calls: +1. Pipes cannot be duplicated (https://github.com/emscripten-core/emscripten/issues/14640) +2. `TemporaryFiles` cannot be duplicated (https://github.com/emscripten-core/emscripten/issues/15012) + +Both of these issues cause trouble with pytest. There is an upstream pull request that would fix this problem: +https://github.com/emscripten-core/emscripten/pull/9396/files + +This patch only partially resolves the problems with `dup` (it doesn't fully duplicate the changes in the emscripten PR) but I think it will be good enough to fix pytest. + +diff --git a/emsdk/upstream/emscripten/src/library_syscall.js b/emsdk/upstream/emscripten/src/library_syscall.js +index 96d2ec0c3..0001624ec 100644 +--- a/emsdk/upstream/emscripten/src/library_syscall.js ++++ b/emsdk/upstream/emscripten/src/library_syscall.js +@@ -137,10 +137,10 @@ var SyscallsLibrary = { + } + return 0; + }, +- doDup: function(path, flags, suggestFD) { ++ doDup: function(stream, suggestFD) { + var suggest = FS.getStream(suggestFD); + if (suggest) FS.close(suggest); +- return FS.open(path, flags, 0, suggestFD, suggestFD).fd; ++ return FS.createStream(stream, suggestFD, suggestFD).fd; + }, + doReadv: function(stream, iov, iovcnt, offset) { + var ret = 0; +@@ -380,7 +380,7 @@ var SyscallsLibrary = { + }, + __sys_dup: function(fd) { + var old = SYSCALLS.getStreamFromFD(fd); +- return FS.open(old.path, old.flags, 0).fd; ++ return FS.createStream(old, 0).fd; + }, + __sys_pipe__deps: ['$PIPEFS'], + __sys_pipe: function(fdPtr) { +@@ -472,7 +472,7 @@ var SyscallsLibrary = { + __sys_dup2: function(oldfd, suggestFD) { + var old = SYSCALLS.getStreamFromFD(oldfd); + if (old.fd === suggestFD) return suggestFD; +- return SYSCALLS.doDup(old.path, old.flags, suggestFD); ++ return SYSCALLS.doDup(old, suggestFD); + }, + __sys_getppid__nothrow: true, + __sys_getppid__proxy: false, +@@ -1167,7 +1167,7 @@ var SyscallsLibrary = { + return -{{{ cDefine('EINVAL') }}}; + } + var newStream; +- newStream = FS.open(stream.path, stream.flags, 0, arg); ++ newStream = FS.createStream(stream, arg); + return newStream.fd; + } + case {{{ cDefine('F_GETFD') }}}: +@@ -1403,7 +1403,7 @@ var SyscallsLibrary = { + assert(!flags); + #endif + if (old.fd === suggestFD) return -{{{ cDefine('EINVAL') }}}; +- return SYSCALLS.doDup(old.path, old.flags, suggestFD); ++ return SYSCALLS.doDup(old, suggestFD); + }, + __sys_pipe2__nothrow: true, + __sys_pipe2__proxy: false, +-- +2.17.1 + diff --git a/src/tests/test_pyodide.py b/src/tests/test_pyodide.py index db8098cd2..9722df6b2 100644 --- a/src/tests/test_pyodide.py +++ b/src/tests/test_pyodide.py @@ -2,6 +2,7 @@ import pytest from pathlib import Path import sys from textwrap import dedent +from pyodide_build.testing import run_in_pyodide sys.path.append(str(Path(__file__).resolve().parents[2] / "src" / "py")) @@ -158,6 +159,41 @@ def test_eval_code_locals(): eval_code("invalidate_caches()", globals, locals) +@run_in_pyodide +def test_dup_pipe(): + # See https://github.com/emscripten-core/emscripten/issues/14640 + import os + + [fdr1, fdw1] = os.pipe() + fdr2 = os.dup(fdr1) + fdw2 = os.dup2(fdw1, 50) + # Closing any of fdr, fdr2, fdw, or fdw2 will currently destroy the pipe. + # This bug is fixed upstream: + # https://github.com/emscripten-core/emscripten/pull/14685 + s1 = b"some stuff" + s2 = b"other stuff to write" + os.write(fdw1, s1) + assert os.read(fdr2, 100) == s1 + os.write(fdw2, s2) + assert os.read(fdr1, 100) == s2 + + +@run_in_pyodide +def test_dup_temp_file(): + # See https://github.com/emscripten-core/emscripten/issues/15012 + import os + from tempfile import TemporaryFile + + tf = TemporaryFile(buffering=0) + fd1 = os.dup(tf.fileno()) + fd2 = os.dup2(tf.fileno(), 50) + s = b"hello there!" + tf.write(s) + # This next assertion actually demonstrates a bug in dup: the correct value + # to return should be b"". + assert os.read(fd1, 50) == s + + @pytest.mark.skip_pyproxy_check def test_monkeypatch_eval_code(selenium): try: