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

Remove unused OS macros from test_unistd_unlink #6142

Merged
merged 1 commit into from
Jan 25, 2018

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Jan 25, 2018

According to this page, emscripten does not predefine any OS-related preprocessor macros other than unix, __unix, and __unix__. There are many apparently unused OS-related macros in the test code (such as __APPLE__ or WIN32), which I think we can delete later.

For this PR, I only deleted them from test_unistd_unlink, which was necessary for #6008 and #6121.

@aheejin
Copy link
Member Author

aheejin commented Jan 25, 2018

@saschanaz

@dschuff dschuff merged commit dc58503 into emscripten-core:incoming Jan 25, 2018
@aheejin aheejin deleted the remove_os_macros branch January 25, 2018 17:48
@juj
Copy link
Collaborator

juj commented Feb 2, 2018

Nooooo... having these kinds of macros is useful when one natively compiles the same file to test for cross-platform differences. Tried to skim through the thread, and was still not sure why this kind of change was needed? Emscripten should never define __APPLE__, but when compiling the same file natively on macOS to test how native GCC/Clang builds behave, having those defines is useful (plus they well document cross-platform differences). Let's not delete __APPLE__ or WIN32 and similar in tests, I add and use them to cross-reference behavior of test code on different platforms.

@saschanaz
Copy link
Collaborator

NODERAWFS depends on native OS behaviors but without OS macros. We had some choices:

  1. Allow different OS behaviors in unconditional way (the current situation)
  2. Manually define OS macros in test runners
  3. Just skip the test 🤕

@aheejin
Copy link
Member Author

aheejin commented Feb 2, 2018

I guess we can just conditionally disable this test on test runners, as I first tried to do in #6121. I'm gonna revert this and reopen #6121 to do that. Any concerns on that?

@saschanaz
Copy link
Collaborator

Disabling tests removes chances to catch and fix bugs...

@dschuff
Copy link
Member

dschuff commented Feb 3, 2018

We could put the OS-specific macros back with their OS-specific expectations, but keep the expectation for emscripten the same as it is here (allowing 2 different behaviors). @juj is just saying the OS-specific macros are not actually unused (because he runs them outside emscripten), so they can stay.

@juj
Copy link
Collaborator

juj commented Feb 5, 2018

Ah thanks, now I understand - executing NODERAWFS under node.js causes different behavior depending on whether the host system is macOS or some other OS. Thanks for restoring the macros in that PR. I think it's good to keep __APPLE__ and __linux__ with the meaning "running natively on macOS/Linux" (and these won't be defined when compiling to target Emscripten). I think we have two choices for the node.js on macOS vs node.js on Linux differences (or is it a node.js on macOS/Linux vs node.js on Windows difference?):

  1. Implement in NODERAWFS explicit detection for macOS and uniformize the different behavior inside NODERAWFS implementation, and pick one as the canonical NODERAWFS behavior.
  2. Use custom flags HOST_APPLE and/or HOST_LINUX that are passed into the build in the test runner suite specifically for the NODERAWFS tests.

A third proposal might be to add Emscripten compile time flags HOST_APPLE and/or HOST_LINUX that were always present when building on the respective systems, but that would make the resulting .js script not migratable/distributable between macOS and Linux systems, so that would be preferable to avoid, and hence keep such flags specific to the test runner.

For NODEFS we do already do something like 1. above and uniformize Windows behavior to look more like Linux/macOS behavior, so to me it's a fine solution, although 2. is fine as well, depending on which direction you'd like to author NODERAWFS towards to. What do you think?

@saschanaz
Copy link
Collaborator

uniformize the different behavior inside NODERAWFS implementation, and pick one as the canonical NODERAWFS behavior.

For NODEFS we do already do something like 1. above and uniformize Windows behavior to look more like Linux/macOS behavior, so to me it's a fine solution

libuv tried it but failed, nodejs/node#18014 for example. The underlying Windows native APIs do not provide enough details to keep consistent behavior. IMO 2. would be the safest option.

@aheejin
Copy link
Member Author

aheejin commented Feb 5, 2018

@juj Just FYI, emscripten does define __linux__. Not __APPLE__ though. See #6121 (comment) for details.

@juj
Copy link
Collaborator

juj commented Feb 9, 2018

@juj Just FYI, emscripten does define linux. Not APPLE though. See #6121 (comment) for details.

Ohh, right.. indeed it does. This is both desirable and undesirable in some cases. It's nice to advertise to behave like Unix/Linux for a lot of scenarios, but for some others, it is not. I think this would be something we'd have two compilation modes for, there's already Emscripten "strict" mode, which might be used for this.

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.

4 participants