Disable NODERAWFS tests on Mac #6121

aheejin · 2018-01-20T00:17:48Z

NODERAWFS support added by #6008 seems to be still flaky on Mac. (Not sure about Windows. Out waterfall bots have other problems at the moment so they can't test this)

Also add support for platform-dependent tests.

saschanaz · 2018-01-20T00:28:13Z

I would say that we add/modify conditional preprocessing on the failing unistd test. I think there already is one...

saschanaz · 2018-01-20T06:40:57Z

https://github.com/kripken/emscripten/blob/941e9de0946077d835a40556048af1de4e4e2abe/tests/unistd/unlink.c#L140-L158

We have a couple of platform guards here, can we fix them instead of just disabling failing tests?

aheejin · 2018-01-21T05:54:14Z

Those #ifdef __APPLE__ guards don't seem to be working for emscripten. Not sure why there are so many of them in the tests/ directory. Emscripten compiler does not predefine __APPLE__.

The reason is, in clang, WebAssembly (for Wasm backend) and Emscripten (for fastcomp) have a separate OS target, so when we give -target wasm32-unknown-unknown-elf (for Wasm backend) or -target asmjs-unknown-emscripten (for fastcomp) to clang, the host OS will not be the actual OS the code is running but a separate "WebAssembly" or "Emscripten" OS.
fastcomp: clang-predefined macros
Wasm backend: clang-predefined macros

Rather, emscripten predefines unix, __unix, and __unix__.
fastcomp: predefined unix macros
Wasm backend: predefined unix macros
#4718 and #4727 argue they add the unix macros because emscripten behaves similarly to unix, but #4718 (comment) also has concerns on if it is strictly accurate.

So, in short, OS-related macros like __APPLE__ do not currently work in emscripten. Do you think we should re-evaluate our macro-defining choices, so we should define macros based on the actual OS the code is running? Or should we just disable the relevant tests as I did in this PR?

Maybe good to cc people in those old issues as well: @kripken @juj @jgravelle-google @dschuff

saschanaz · 2018-01-21T07:24:39Z

Hmm, we may have to accept multiple error numbers as expected without conditions.

assert(errno == EISDIR || errno == EPERM); // Linux throws EISDIR, macOS throws EPERM (for example)

dschuff · 2018-01-22T16:59:16Z

Emscripten shouldn't define OS-dependent things like __APPLE__, it should behave the same regardless of the host system. Because of course the underlying target environments (Web, Node) are supposed to also behave the same. If I understand NODERAWFS correctly we are basically drilling a hole into the underlying platform and breaking that abstraction, so I suppose we really are asking for this kind of trouble.
So I guess we should more precisely decide what the goal of the API is; either

we want to provide our own low-level platform-independent API. In this case we should still hide the platform from the user and the goal should be to have all tests working the same on all platforms, and the API will need to be least-common-denominator. Or,
the point is to expose the uniqueness of the underlying platform, in which case we will of course have different tests for each. (Even in that case I think the platform should only be exposed via the API and not via the compiler).

Practically speaking, even if we want to do 1, we may or may not need a way for the API's implementation to tell what platform it's on, but it would be hidden from the user.

Short-term, it probably makes sense to disable the tests wherever they don't work for now.

aheejin · 2018-01-22T19:36:58Z

There are many conditional guards like #ifdef __APPLE__ based on the OS-predefined symbols in the emscripten test code. I wonder what they tried to target in the first place then. Do you think we should remove them?

dschuff · 2018-01-22T19:44:54Z

In principle they probably shouldn't be there, and in practice they don't do anything, so it seems like they should probably be removed, (depending on what they are there for, e.g. if they came from third-party code or something).

As for this PR specifically, do we want to duplicate or replace the OS detection logic from shared.py? I get that we need no_windows() etc to fit in with the skipping logic but maybe we should leave the if WINDOWS bits in this file as they are, and (if we want to replace that) replace it globally in a separate PR.

aheejin · 2018-01-22T19:55:37Z

@dschuff The WINDOWS thing from shared.py is not completely replaced; it is used elsewhere too. I made is_mac() and is_windows() to allow us to use decorators like @no_mac and @no_windows, and thought it would make more sense to consistently use them in this test_core.py.

We have three options:

Add @no_mac / @no_windows / is_mac() / is_windows(), and use them wherever it is necessary within test_core.py. (The current state) WINDOWS from shared.py is also used elsewhere, so don't touch that. (I'm not sure what you meant by 'globally'. I don't think is_windows() can be used outside of this file.)
Add @no_mac / @no_windows / is_mac() / is_windows(), but only use them as decorators like @no_mac, and when we want to test the condition in an if statement, we stick to WINDOWS or OSX defined in shared.py.
Don't add anything, and disable tests only with WINDOWS or OSX. That means even when we want to disable a test as a whole, we need to insert ifs within the function to use WINDOWS/OSX.

If we go with the option 1 as I am currently doing, I think separating it with two PRs might make more sense too, as you suggested:

Add @no_mac / @no_windows / is_mac() / is_windows(), and replace uses within this file to use them
Disable NODERAWFS-related tests on Mac

kripken · 2018-01-22T20:59:18Z

Is the issue here just the error codes? If so, can we do what @saschanaz suggested above,

assert(errno == EISDIR || errno == EPERM); // Linux throws EISDIR, macOS throws EPERM (for example)

or is the OS-specific behavior more complicated to work around in the test?

aheejin · 2018-01-22T22:36:17Z

@kripken If we are already disabling windows by this code, wouldn't disabling mac in the same manner, within this if statement, be more consistent?
https://github.com/kripken/emscripten/blob/e24661497247ecac78166b02e25e880e0ee14c9d/tests/test_core.py#L4711-L4716

aheejin · 2018-01-22T22:53:50Z

Split decorator part in #6128, which should land first.

kripken · 2018-01-22T23:21:31Z

Yeah, if we already have an if for Windows, we can add Mac to there too.

saschanaz · 2018-01-23T00:46:36Z

I skipped Windows because permission bits are simply not supported on Windows and all chmod tests were broken.

saschanaz · 2018-01-23T00:53:06Z

BTW, can we file an issue on Nodejs (or possibly libuv) side?

======================================================================
ERROR: test_fs_writeFile (test_core.binaryen2)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 331, in run
    testMethod()
  File "/b/build/slave/mac/build/src/src/work/wasm-install/emscripten/tests/test_core.py", line 57, in decorated
    func(self, js_engines=[NODE_JS])
  File "/b/build/slave/mac/build/src/src/work/wasm-install/emscripten/tests/test_core.py", line 4551, in test_fs_writeFile
    self.do_run_from_file(src, out, js_engines=js_engines)
  File "/b/build/slave/mac/build/src/src/work/wasm-install/emscripten/tests/runner.py", line 649, in do_run_from_file
    self.do_run(open(src).read(), open(expected_output).read(), *args, **kwargs)
  File "/b/build/slave/mac/build/src/src/work/wasm-install/emscripten/tests/runner.py", line 689, in do_run
    raise e
Exception: Expected to find 'read a=1
read b=2
read c=3
' in 'read c=3
read b=2
read 
', diff:
--- expected
+++ actual
@@ -1,4 +1,4 @@
-read a=1
+read c=3
 read b=2
-read c=3
+read

It seems fs.open(src, 'a') doesn't set stream position correctly on Node.js side (and only on macOS), I think it's worth filing an issue then as other users may see the same problem later.

(Unfortunately I don't have any macOS access)

aheejin · 2018-01-23T01:09:48Z

@saschanaz Are you referring to me in regards to the Nodejs issue?

saschanaz · 2018-01-23T01:49:01Z

Yes, but feel free to ignore if you don't have free time or you only have bot access for macOS tests...

aheejin · 2018-01-23T02:19:53Z

Sorry, I don't have much background on Node, so I'm not sure if I can provide sufficient context there.

saschanaz · 2018-01-23T02:22:37Z

Oops, never mind then, sorry!

curiousdannii · 2018-01-23T02:41:25Z

@saschanaz That's documented at https://nodejs.org/api/fs.html#fs_fs_open_path_flags_mode_callback:

On Linux, positional writes don't work when the file is opened in append mode. The kernel ignores the position argument and always appends the data to the end of the file.

You need to create the file if it doesn't exist, open as 'r+', and then manually seek to the end.

[Edit]: oh wait, you're talking about MacOS. Maybe it's the same though?

saschanaz · 2018-01-23T02:54:45Z

IMO that 'positional writes' means giving explicit position argument, whereas our writeFile test expects the append mode automatically sets the position to the EOF.

kripken · 2018-01-23T20:55:20Z

tests/test_core.py

@@ -4564,6 +4564,7 @@ def test_fs_trackingdelegate(self):
    out = path_from_root('tests', 'fs', 'test_trackingdelegate.out')
    self.do_run_from_file(src, out)

+  @no_mac('NODERAWFS support on mac is flaky')


this also disables the non-NODERAWFS part on mac. i guess there isn't a simple way to disable just that part, though, unless we split up this test. not sure it's important enough to justify that.

I split the test into two.

kripken · 2018-01-23T21:56:01Z

tests/test_core.py

+  # disable test_fs_writeFile test, so we split the test into two.
+  # See issue 6121
+  @no_osx('NODERAWFS support on OSX is flaky')
+  @also_with_noderawfs


I think that on non-mac this will run the test twice without NODERAWFS and once with it? (since test_fs_writeFile_noderawfs is marked as also_with_noderawfs which will run it once normally and once with NODERAWFS)

Seems a little wasteful, but there isn't a good and obvious way to fix it. Best I can think of is: go back to what we had before, and in the test check if mac and is_noderawfs, and if so skip that test (I think we need to add a method to check is_noderawfs).

Not sure if I can check that, because I guess the way to implement is_noderawfs would be to check if Settings.NODERAWFS == 1. But that is true only in the child process this test_core.py launches with option -s NODERAWFS=1 and not in test_core.py.

Hmm, yes, I think we would need to check something like 'NODERAWFS=1' in self.emcc_args.

saschanaz · 2018-01-24T00:24:21Z

tests/test_core.py

@@ -4566,6 +4566,9 @@ def test_fs_trackingdelegate(self):

  @also_with_noderawfs
  def test_fs_writeFile(self, js_engines=None):
+    if self.is_osx() and 'NODERAWFS=1' in self.emcc_args:


Too bad to miss reviewing #6128, but can we rename is_osx to is_macos, as Apple renamed OS X to macOS to keep consistency with iOS, watchOS and tvOS?

Done in #6138, which should land first.

saschanaz · 2018-01-25T01:32:48Z

Now the only thing left is the unlink test. Can you check what errno the test emits on line 90 of tests/unistd/unlink.c, and if possible allow it like this?

assert(errno == EISDIR || errno == EPERM); // Linux throws EISDIR, macOS throws EPERM (for example)

aheejin · 2018-01-25T02:35:30Z

Even if we do that, the assertion on the line 157 fails too; the errno in this case is EISDIR.
https://github.com/kripken/emscripten/blob/16df331292e156d5578c053c26bc73943904d6d0/tests/unistd/unlink.c#L152-L158

It would look kind of weird if we change that code into

#ifdef __APPLE__
  assert(errno == EISDIR);
#else
  assert(errno == EBUSY || EISDIR);
#endif

when there is no case the ifdef __APPLE__ part can be satisfied.

How about wrapping up this PR here and deleting all __APPLE__ related code in another PR?

saschanaz · 2018-01-25T04:18:50Z

Maybe someone wanted to thoroughly check native gcc/clang behavior, but 👍 for removing __APPLE__ codes and unconditionally allowing all OS-dependent behaviors.

aheejin · 2018-01-25T17:53:42Z

After #6140 and #6142 are merged, this should become unnecessary. I'll check the waterfall until it goes back to green and then abandon this if everything's OK.

aheejin added 2 commits January 19, 2018 16:13

Disable NODERAWFS tests on Mac

46f52b9

Also add support for platform-dependent tests.

uppercase

deef0c6

aheejin added 2 commits January 19, 2018 16:35

Use is_mac() and is_windows()

869207f

typo

e41f2e4

Merge branch 'incoming' into disable_noderawfs_on_mac

1d8560f

kripken reviewed Jan 23, 2018

View reviewed changes

Split the test into two

6e10c36

kripken reviewed Jan 23, 2018

View reviewed changes

address comments

872c6c7

saschanaz reviewed Jan 24, 2018

View reviewed changes

aheejin added 2 commits January 24, 2018 11:09

Merge branch 'incoming' into disable_noderawfs_on_mac

5d155e9

OSX -> macOS

a2ee62a

aheejin mentioned this pull request Jan 24, 2018

NODERAWFS writeFile append mode fix for macOS #6140

Merged

Re-enable test_fs_writeFile

657f5fc

aheejin mentioned this pull request Jan 25, 2018

Remove unused OS macros from test_unistd_unlink #6142

Merged

aheejin closed this Jan 30, 2018

aheejin deleted the disable_noderawfs_on_mac branch February 3, 2018 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable NODERAWFS tests on Mac #6121

Disable NODERAWFS tests on Mac #6121

aheejin commented Jan 20, 2018

saschanaz commented Jan 20, 2018

saschanaz commented Jan 20, 2018

aheejin commented Jan 21, 2018

saschanaz commented Jan 21, 2018 •

edited

Loading

dschuff commented Jan 22, 2018 •

edited

Loading

aheejin commented Jan 22, 2018

dschuff commented Jan 22, 2018

aheejin commented Jan 22, 2018 •

edited

Loading

kripken commented Jan 22, 2018

aheejin commented Jan 22, 2018 •

edited

Loading

aheejin commented Jan 22, 2018

kripken commented Jan 22, 2018

saschanaz commented Jan 23, 2018

saschanaz commented Jan 23, 2018 •

edited

Loading

aheejin commented Jan 23, 2018

saschanaz commented Jan 23, 2018

aheejin commented Jan 23, 2018

saschanaz commented Jan 23, 2018

curiousdannii commented Jan 23, 2018 •

edited

Loading

saschanaz commented Jan 23, 2018 •

edited

Loading

kripken Jan 23, 2018

aheejin Jan 23, 2018

kripken Jan 23, 2018

aheejin Jan 23, 2018

kripken Jan 23, 2018

aheejin Jan 23, 2018

saschanaz Jan 24, 2018

aheejin Jan 24, 2018 •

edited

Loading

saschanaz commented Jan 25, 2018

aheejin commented Jan 25, 2018 •

edited

Loading

saschanaz commented Jan 25, 2018 •

edited

Loading

aheejin commented Jan 25, 2018

Disable NODERAWFS tests on Mac #6121

Disable NODERAWFS tests on Mac #6121

Conversation

aheejin commented Jan 20, 2018

saschanaz commented Jan 20, 2018

saschanaz commented Jan 20, 2018

aheejin commented Jan 21, 2018

saschanaz commented Jan 21, 2018 • edited Loading

dschuff commented Jan 22, 2018 • edited Loading

aheejin commented Jan 22, 2018

dschuff commented Jan 22, 2018

aheejin commented Jan 22, 2018 • edited Loading

kripken commented Jan 22, 2018

aheejin commented Jan 22, 2018 • edited Loading

aheejin commented Jan 22, 2018

kripken commented Jan 22, 2018

saschanaz commented Jan 23, 2018

saschanaz commented Jan 23, 2018 • edited Loading

aheejin commented Jan 23, 2018

saschanaz commented Jan 23, 2018

aheejin commented Jan 23, 2018

saschanaz commented Jan 23, 2018

curiousdannii commented Jan 23, 2018 • edited Loading

saschanaz commented Jan 23, 2018 • edited Loading

kripken Jan 23, 2018

Choose a reason for hiding this comment

aheejin Jan 23, 2018

Choose a reason for hiding this comment

kripken Jan 23, 2018

Choose a reason for hiding this comment

aheejin Jan 23, 2018

Choose a reason for hiding this comment

kripken Jan 23, 2018

Choose a reason for hiding this comment

aheejin Jan 23, 2018

Choose a reason for hiding this comment

saschanaz Jan 24, 2018

Choose a reason for hiding this comment

aheejin Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

saschanaz commented Jan 25, 2018

aheejin commented Jan 25, 2018 • edited Loading

saschanaz commented Jan 25, 2018 • edited Loading

aheejin commented Jan 25, 2018

saschanaz commented Jan 21, 2018 •

edited

Loading

dschuff commented Jan 22, 2018 •

edited

Loading

aheejin commented Jan 22, 2018 •

edited

Loading

aheejin commented Jan 22, 2018 •

edited

Loading

saschanaz commented Jan 23, 2018 •

edited

Loading

curiousdannii commented Jan 23, 2018 •

edited

Loading

saschanaz commented Jan 23, 2018 •

edited

Loading

aheejin Jan 24, 2018 •

edited

Loading

aheejin commented Jan 25, 2018 •

edited

Loading

saschanaz commented Jan 25, 2018 •

edited

Loading