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

Fix half of emscripten's failing tests #31532

Merged
merged 2 commits into from Feb 11, 2016

Conversation

Projects
None yet
5 participants
@tomaka
Contributor

tomaka commented Feb 10, 2016

Before this PR:

test result: FAILED. 2039 passed; 327 failed; 2 ignored; 0 measured

After:

test result: FAILED. 2232 passed; 134 failed; 2 ignored; 0 measured

r? @brson

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Feb 10, 2016

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Feb 10, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

// For targets like MUSL or Emscripten, however, there is no support for
// dynamic libraries so we just go back to building a normal library. Note,
// however, that for MUSL if the library is built with `force_host` then
// it's ok to be a dylib as the host should always support dylibs.

This comment has been minimized.

@oli-obk

oli-obk Feb 10, 2016

Contributor

why isn't the force_host statement also true for emscripten?

@oli-obk

oli-obk Feb 10, 2016

Contributor

why isn't the force_host statement also true for emscripten?

This comment has been minimized.

@tomaka

tomaka Feb 10, 2016

Contributor

The comment says if the library is built with force_host then it's ok to be a dylib as the host should always support dylibs, but emscripten doesn't support dynamic libraries at all (the "libraries" are just hidden javascript files).

@tomaka

tomaka Feb 10, 2016

Contributor

The comment says if the library is built with force_host then it's ok to be a dylib as the host should always support dylibs, but emscripten doesn't support dynamic libraries at all (the "libraries" are just hidden javascript files).

This comment has been minimized.

@oli-obk

oli-obk Feb 10, 2016

Contributor

yes, but the host can never be emscripten, right? That would mean we'd have a rustc that runs on emscripten.

I think force_host is just for rustc's unit tests when compiling a compiler plugin.

@oli-obk

oli-obk Feb 10, 2016

Contributor

yes, but the host can never be emscripten, right? That would mean we'd have a rustc that runs on emscripten.

I think force_host is just for rustc's unit tests when compiling a compiler plugin.

This comment has been minimized.

@tomaka

tomaka Feb 10, 2016

Contributor

That would mean we'd have a rustc that runs on emscripten.

Well, why not? 🐷

@tomaka

tomaka Feb 10, 2016

Contributor

That would mean we'd have a rustc that runs on emscripten.

Well, why not? 🐷

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Feb 10, 2016

Contributor

@bors r+ Thanks @tomaka!

Contributor

brson commented Feb 10, 2016

@bors r+ Thanks @tomaka!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Feb 10, 2016

Contributor

📌 Commit 657f1cf has been approved by brson

Contributor

bors commented Feb 10, 2016

📌 Commit 657f1cf has been approved by brson

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Feb 11, 2016

Contributor

⌛️ Testing commit 657f1cf with merge aa1dc09...

Contributor

bors commented Feb 11, 2016

⌛️ Testing commit 657f1cf with merge aa1dc09...

bors added a commit that referenced this pull request Feb 11, 2016

Auto merge of #31532 - tomaka:fix-emscripten, r=brson
Before this PR:

> test result: FAILED. 2039 passed; 327 failed; 2 ignored; 0 measured

After:

> test result: FAILED. 2232 passed; 134 failed; 2 ignored; 0 measured

r? @brson

@bors bors merged commit 657f1cf into rust-lang:master Feb 11, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@tomaka tomaka deleted the tomaka:fix-emscripten branch Feb 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment