Skip to content

Fix half of emscripten's failing tests #31532

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

Merged
merged 2 commits into from
Feb 11, 2016
Merged

Conversation

tomaka
Copy link
Contributor

@tomaka 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
Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Well, why not? 🐷

@brson
Copy link
Contributor

brson commented Feb 10, 2016

@bors r+ Thanks @tomaka!

@bors
Copy link
Collaborator

bors commented Feb 10, 2016

📌 Commit 657f1cf has been approved by brson

@bors
Copy link
Collaborator

bors commented Feb 11, 2016

⌛ Testing commit 657f1cf with merge aa1dc09...

bors added a commit that referenced this pull request Feb 11, 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
@bors bors merged commit 657f1cf into rust-lang:master Feb 11, 2016
@tomaka tomaka deleted the fix-emscripten branch February 11, 2016 18:58
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.

5 participants