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

Disable multithreading for all wasm32 targets. #71

Merged
merged 1 commit into from
Feb 10, 2018

Conversation

BartAdv
Copy link
Contributor

@BartAdv BartAdv commented Feb 4, 2018

With the coming of wasm32-unknown-unknown it might be good idea to use target_arch = "wasm32" to detect whether to build with multithreading support or not.

@torkleyy
Copy link
Member

torkleyy commented Feb 4, 2018

Thanks for your contribution :)

I would prefer to change this to a parallel feature, as you already said in slide-rs/specs#341.

Copy link
Member

@WaDelma WaDelma left a comment

Choose a reason for hiding this comment

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

I would also like having parallel feature instead of all this wasm cfgs as there are probably other reasons for disabling threading.

@@ -239,7 +239,8 @@ impl<'a, 'b> DispatcherBuilder<'a, 'b> {
/// Same as
/// [`add_pool()`](struct.DispatcherBuilder.html#method.add_pool),
/// but returns `self` to enable method chaining.
#[cfg(not(target_os = "emscripten"))]
#[cfg(not(target_arch = "wasm32"))]

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove empty line

@BartAdv
Copy link
Contributor Author

BartAdv commented Feb 4, 2018

Yeah, I'm all open for suggestions - I just wasn't sure what's the so called "best practice" approach would be. In general, you're right, there might be more reasons to disable multithreading than the target itself, but on the other hand, in this case (wasm32) target itself determine that you just can't have multithreading...

On the other hand, here we not only have multithreading, we also have async (pulse) stuff...

@BartAdv
Copy link
Contributor Author

BartAdv commented Feb 4, 2018

Let's maybe coordinate it here: amethyst/specs#341 (comment)

@BartAdv
Copy link
Contributor Author

BartAdv commented Feb 4, 2018

I've altered the conditionals to use "parallel" feature.

@BartAdv
Copy link
Contributor Author

BartAdv commented Feb 4, 2018

Hmm, what's up with the versions though? master is 0.5.0, when I wanted to adjust specs to use the version from this PR, I noticed failures when running tests, and I noticed the version it picked up was 0.5.2, which lives in the v0.5 branch, which is a bit ahead, a bit behind the master...

Copy link
Member

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

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

Oh, I forgot about this PR, sorry. Thank you for the contribution!

@torkleyy
Copy link
Member

Hmm, what's up with the versions though? master is 0.5.0, when I wanted to adjust specs to use the version from this PR, I noticed failures when running tests, and I noticed the version it picked up was 0.5.2, which lives in the v0.5 branch, which is a bit ahead, a bit behind the master...

This branch is essentially the future 0.6 version, just didn't bump the number in Cargo.toml yet.

bors r+

bors bot added a commit that referenced this pull request Feb 10, 2018
71: Disable multithreading for all wasm32 targets. r=torkleyy a=BartAdv

With the coming of `wasm32-unknown-unknown` it might be good idea to use `target_arch = "wasm32"` to detect whether to build with multithreading support or not.
@bors
Copy link
Contributor

bors bot commented Feb 10, 2018

Build succeeded

@bors bors bot merged commit fd2536e into amethyst:master Feb 10, 2018
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.

3 participants