add parallel flag depending on cmake version. As well as MP flag for VS #107
Conversation
| @@ -718,7 +718,7 @@ impl Config { | |||
| parallel_flags = Some(format!("-j{}", s)); | |||
| } | |||
| Some(ref g) if g.contains("Visual Studio") => { | |||
| parallel_flags = Some(format!("/m:{}", s)); | |||
| parallel_flags = Some(format!("/MP{}", s)); | |||
There was a problem hiding this comment.
To confirm, you've seen this have parallelized effects locally for yourself? I'm not sure how cmake invokes cl.exe but it looks like this only works if cl.exe is fed multiple files at once? I'm also not sure what the previous /m option was actually...
There was a problem hiding this comment.
Yes. I haven't come across the m flag before personally.
src/lib.rs
Outdated
| } else { | ||
| let cmake_version = cmake_version(); | ||
| if cmake_version.0 >= 3 && cmake_version.1 > 12 { | ||
| parallel_flags = Some(String::from("--parallel")); |
There was a problem hiding this comment.
Is --parallel super new? Ideally we wouldn't do a version check since that's an extra process and can take some time, but it's not the end of the world.
There was a problem hiding this comment.
Also, to clarify, I think this only gets run outside of Cargo's build scripts since Cargo should always set NUM_JOBS, but are you using the cmake crate in that style?
There was a problem hiding this comment.
Since cmake 3.12. I use it the cmake crate with 2 other crates of mine. fltk-rs and soloud-rs. For soloud, the c++ code base is small so it builds fast. But with fltk I had noticed slower build times when building on windows-msvc. Manually passing NUM_JOBS didn't seem to help much. So I think it's the m flag thing.
src/lib.rs
Outdated
| let version = String::from_utf8_lossy(&cmd.stdout); | ||
| let version: Vec<&str> = version.split_whitespace().collect(); | ||
| let version: Vec<&str> = version[2].split(".").collect(); | ||
| let version: Vec<u8> = version.iter().map(|c| c.parse().unwrap()).collect(); |
There was a problem hiding this comment.
I think these .unwrap() will want to go away and we'll want to return an Option from this function to avoid panicking if something goes wrong here.
There was a problem hiding this comment.
I think I can remove this if things work out with the MP flag.
Looking at the microsoft docs, the m flag doesn't seem to be listed:
https://docs.microsoft.com/en-us/cpp/build/reference/compiler-options-listed-alphabetically?view=msvc-160
|
Ok looks reasonable to me, thanks! |
No description provided.