Skip to content

/rust

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

More emscripten test fixes #31623

Closed
wants to merge 3 commits into from

Conversation

Reviewers
No reviews
Assignees

@brson brson

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@tomaka @brson @alexcrichton
@tomaka
Copy link
Contributor

tomaka commented Feb 13, 2016

r? @brson

  • libtest now runs each test in single-threaded mode (around panic::recover) if the test thread fails to spawn. This is required since emscripten doesn't have threads.
  • Previously linking errors only triggered warnings, and missing symbols were replaced with abort(-1). I think it's better to always error when a symbol is missing.
  • DISABLE_EXCEPTION_CATCHING=0 is required to make exceptions work when optimizations are on. However the docs say that this causes a big difference in binary size and runtime performances. From my experience with emscripten, these differences can sometimes be massive.
  • All tests that rely on spawning a new process have been ignored, as this is not supported by emscripten.

I also have some modifications in my local clone of the emscripten SDK to make all this work.
@tomaka
More emscripten test fixes
Loading status checks…
5af4c0c

@rust-highfive rust-highfive assigned brson Feb 13, 2016

src/libtest/lib.rs
let test_result = calc_result(&desc2, test_result);
let stdout = data3.lock().unwrap().to_vec();
monitor_ch2.send((desc2.clone(), test_result, stdout)).unwrap();
}

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Feb 13, 2016

Member

Perhaps this could become the way tests are run? It'd be good to exercise this all the time rather than just with emscripten.

This comment has been minimized.

Sign in to view
Copy link
@tomaka

tomaka Feb 13, 2016

Author Contributor

But then tests would execute one at a time instead of concurrently.

This comment has been minimized.

Sign in to view
Copy link
@brson

brson Feb 13, 2016

Contributor

We don't want to reuse threads for tests generally because of TLS.

This comment has been minimized.

Sign in to view
Copy link
@brson

brson Feb 13, 2016

Contributor

It does look like a lot of duplication for a path that won't be exercised much. Can any of it be factored out?

This comment has been minimized.

Sign in to view
Copy link
@tomaka

tomaka Feb 14, 2016

Author Contributor

I don't think it's possible to reduce it, except for some minor things.
It would probably be feasible if Builder::spawn() returned the closure that was supposed to be called in the case of an error.
@tomaka

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

tomaka commented Feb 13, 2016

I pushed a second minor commit that finishes ignoring all tests.
@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Feb 13, 2016

Can you link your emscripten changes? Are they likely to be accepted upstream?
@tomaka

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

tomaka commented Feb 13, 2016

I submitted emscripten-core/emscripten#4097 and emscripten-core/emscripten#4095. The PRs may need some tweaks, but I think the nature of the changes is probably good.

I also have some preliminary changes that add proper support for unwinding, but it needs more work before I submit it.
@tomaka
More test ignores
Loading status checks…
876c8bc
@tomaka

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

tomaka commented Feb 28, 2016

Remain to be fixed:
@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Mar 3, 2016

@tomaka Can we change the test runner code to use conditional compilation to detect emscripten and run single-threaded? That way we don't have this never-executed failure path on other platforms.

@tomaka tomaka referenced this pull request Apr 29, 2016

Closed

Make Rust work with emscripten #604

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented May 1, 2016

Closing due to inactivity, but feel free to resubmit with a rebase!

@alexcrichton alexcrichton closed this May 1, 2016

badboy added a commit to badboy/rust that referenced this pull request Aug 8, 2016

@badboy
[emscripten] Ignore tests 
Most of these rely on spawning processes, which is not possible in
Emscripten.

Based on rust-lang#31623
2e80534

@badboy badboy referenced this pull request Aug 10, 2016

Merged

Emscripten test fixes #35574

eddyb added a commit to eddyb/rust that referenced this pull request Aug 14, 2016

@eddyb
Rollup merge of rust-lang#35574 - badboy:emscripten-test-fixes, r=brson 
Emscripten test fixes

This picks up parts of rust-lang#31623 to disable certain tests that emscripten can't run, as threads/processes are not supported.
I re-applied @tomaka's changes manually, I can rebase those commits with his credentials if he wants.

It also disables jemalloc for emscripten (at least in Rustbuild, I have to check if there is another setting for the same thing in the old makefile approach).

This should not impact anything for normal builds.
524bdb5

eddyb added a commit to eddyb/rust that referenced this pull request Aug 14, 2016

@eddyb
Rollup merge of rust-lang#35574 - badboy:emscripten-test-fixes, r=brson 
Emscripten test fixes

This picks up parts of rust-lang#31623 to disable certain tests that emscripten can't run, as threads/processes are not supported.
I re-applied @tomaka's changes manually, I can rebase those commits with his credentials if he wants.

It also disables jemalloc for emscripten (at least in Rustbuild, I have to check if there is another setting for the same thing in the old makefile approach).

This should not impact anything for normal builds.
8ade28e

@tomaka tomaka deleted the tomaka:even-more-emscripten branch Dec 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.