Fixed build failures of simd feature on the latest rust nightly #98
Conversation
rust-toolchain.toml
Outdated
| @@ -0,0 +1,4 @@ | |||
| [toolchain] | |||
There was a problem hiding this comment.
I don't think we would want to have nightly toolchain by default. Instead, I think it would be better to add a justfile recipe to compile simd version -- and pass all the needed parameters via the command line.
Cargo.toml
Outdated
|
|
||
| [dependencies.packed_simd] | ||
| git = "https://github.com/programingjd/packed_simd.git" | ||
| default-features = false | ||
| features = ["into_bits"] |
There was a problem hiding this comment.
Can we put these inline inside the [dependencies] section? Makes it more readable IMO, vs looking at a dedicated section. Also, this will need to be tied to a specific git revision (or at least a tag or a branch).
| [dependencies.packed_simd] | |
| git = "https://github.com/programingjd/packed_simd.git" | |
| default-features = false | |
| features = ["into_bits"] | |
| "packed_simd" = { default-features = false, features = ["into_bits"], git = "https://github.com/programingjd/packed_simd.git" } |
src/enc/worker_pool.rs
Outdated
| @@ -132,7 +132,7 @@ impl <ReturnValue:Send+'static, | |||
| if local_queue.shutdown{ | |||
| break; | |||
| } else { | |||
| let _ = cvar.wait(local_queue); // unlock immediately, unfortunately | |||
| let _lock = cvar.wait(local_queue); // unlock immediately, unfortunately | |||
There was a problem hiding this comment.
why do you need a named var?
There was a problem hiding this comment.
Because if the variable isn't named, we get this error:
error: non-binding let on a synchronization lock
--> src\enc\worker_pool.rs:135:19
|
135 | let _ = cvar.wait(local_queue); // unlock immediately, unfortunately
| ^ this lock is not assigned to a binding and is immediately dropped
There was a problem hiding this comment.
Thanks! Does it conflict with the comment then? Since we don't unlock immediately? I think the comment may need an update/explanation
There was a problem hiding this comment.
I think the comment still makes sense since the lock is dropped right after the continue on next line, isn't it?
|
why closing? |
|
Sorry, I rebased and then I didn't know how to update the pull request. I tried to sync my fork and it auto-closed the pull request. |
|
Weird - usually you just do a |
|
I've opened a new pull request. Sorry about the mess. |
|
Continued in #140 @programingjd note that you can also submit a PR against https://github.com/rust-brotli/rust-brotli -- this way it will auto-build with CI Github allows you to create a PR from the same branch against multiple repos |
The simd feature requires nightly to build, but it doesn't build on the latest nightly because of the dependency on packed_simd_2.
There's an issue on the packed_simd repo related to this build failure: Issue 356
I have applied the suggested changes to fix the build: replaced the dependency on packed_simd_2 0.3.8 with a dependency on packed_simd 0.3.9.