Fix querying emcc on windows

[guusw]

Pretty minor change, just queries emcc.bat since emcc will not work on windows

For example this simple script will output this on windows

fn main() {
    let output = std::process::Command::new("emcc.bat").arg("-dumpversion").output().ok();
    println!("{:?}", output);
    let output1 = std::process::Command::new("emcc").arg("-dumpversion").output().ok();
    println!("{:?}", output1);
}

rustc test.rs && ./test.exe
Some(Output { status: ExitStatus(ExitStatus(0)), stdout: "3.1.74\r\n", stderr: "" })
None

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[tgross35]

Nit: could you use if cfg!(target_os = "windows") { ... } here instead? cfg!(...) is slightly preferred over #[cfg(...)] where possible since it allows the code to be checked on all OSs.

Otherwise lgtm, please squash with the change.

Fyi @hoodmane.

[guusw]

Applied this

[guusw]

Could this go on 0.2 as well once it's merged?

[hoodmane]

Cc @messense maybe we want this change in maturin too?

[messense]

Cc @messense maybe we want this change in maturin too?

Done in PyO3/maturin#2478

[tgross35]

Could this go on 0.2 as well once it's merged?

Fyi anyone can add the label, it's just

@rustbot label +stable-nominated

[tgross35]

Looks like this needs a rebase, could you do that?

[tgross35]

@rustbot author for the rebase

[tgross35]

Actually this could be cleaned up a bit, we already have Command imported and the two commands are identical except for the executable name:

    let emcc = if cfg!(target_os = "windows") {
        "emcc.bat"
    } else {
        "emcc"
    };

    Command::new(emcc).arg("-dumpversion").output()?;

[tgross35]

@hoodmane we haven't heard from the author in a while, any chance you just want to rebase this yourself (plus the one change above) and submit a new PR?

Or @guusw if you're able to rebase this.

[hoodmane]

Yeah I will do that when I have a moment.

[guusw]

Ah sorry I haven't looked at this in a while. Actually I revised it slightly locally because I realized It doesn't really make sense to silently set version to None in case emcc is not found, so having an error in this case is better I think.
I rebased and made that change a separate commit so can be checked by itself.

It would work something like this

/c/Projects/libc>cargo rustc --target-dir=./t -r --crate-type=lib --target=wasm32-unknown-emscripten                                                                                                                                                 (fix-emcc-main↑1|c1…1) 
   Compiling libc v1.0.0-alpha.1 (C:\Projects\libc)
error: failed to run custom build command for `libc v1.0.0-alpha.1 (C:\Projects\libc)`

Caused by:
  process didn't exit successfully: `C:\Projects\libc\./t\release\build\libc-6eb23dab9665ec17\build-script-build` (exit code: 101)
  --- stdout
  cargo:rerun-if-changed=build.rs
  cargo:rerun-if-env-changed=RUST_LIBC_UNSTABLE_FREEBSD_VERSION
  cargo:rustc-cfg=freebsd12

  --- stderr
  thread 'main' panicked at build.rs:228:13:
  Failed to query emcc for version, make sure emcc is installed
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
/c/Projects/libc>export PATH=$PATH:/a/Projects/emsdk:/a/Projects/emsdk/upstream/emscripten                                                                                                                                                           (fix-emcc-main↑1|c1…2) 
/c/Projects/libc>cargo rustc --target-dir=./t -r --crate-type=lib --target=wasm32-unknown-emscripten                                                                                                                                                 (fix-emcc-main↑1|c1…2) 
   Compiling libc v1.0.0-alpha.1 (C:\Projects\libc)
    Finished release [optimized] target(s) in 0.40s
/c/Projects/libc>    

EDIT: so I also added the RUST_LIBC_UNSTABLE_EMCC_VERSION=30142 variable as a workaround for when emscripten is somehow not installed but you're building for it anyways (like in the CI runs)

[tgross35]

Well you updating makes things easier, thanks :)

Panicing isn't really an option here unfortunately, we need to just pick a suitable default version. Otherwise that breaks anyone who might be running cargo check on emscripten targets but doesn't actually have emcc installed, which currently works.

[guusw]

Hmm yeah I see how that could break cargo check (specifically cargo check --target=wasm32-unknown-emscripten)
My only annoyance is that if this is setup incorrectly it would basically silently compile against an incorrect ABI causing random issues down the line.
Not having a panic is probably okay because the default is the newer ABI so when using the latest emscripten it should never be an issue. I'll revert that change

[tgross35]

Thanks for the updates!

[tgross35]

Not having a panic is probably okay because the default is the newer ABI so when using the latest emscripten it should never be an issue. I'll revert that change

I think this is probably unlikely because on machines where this will be linking far enough that it matters, emcc is likely installed. However, if emscripten allows breaking ABI changes at regular releases, it may be worth splitting the target into multiple that have a version differentiator. Currently this is only done for qnx, but we would like to do it for others (like OpenBSD) https://doc.rust-lang.org/nightly/rustc/platform-support.html.