cmake-rs icon indicating copy to clipboard operation
cmake-rs copied to clipboard

Suggestion: Windows CI

Open rjsberry opened this issue 7 years ago • 14 comments

Partially related to #28.

Would you be receptive to a PR adding Windows CI? I'm in the process of submitting a PR to FlatBuffers and I'm seeing what looks like invalid generated cmake commands on their AppVeyor CI jobs:

--- stdout
running: "cmake" "C:\\projects\\flatbuffers\\rust\\flatc\\..\\.." "-Thost=x64" "-DCMAKE_GENERATOR_PLATFORM=x64" "-G" "Visual Studio 10 2010" "-DCMAKE_BUILD_TYPE=Release" "-DCMAKE_INSTALL_BINDIR=bin" "-DFLATBUFFERS_BUILD_TESTS=OFF" "-DFLATBUFFERS_BUILD_FLATLIB=OFF" "-DFLATBUFFERS_BUILD_FLATHASH=OFF" "-DCMAKE_INSTALL_PREFIX=C:\\projects\\flatbuffers\\tests\\rust_usage_test\\target\\debug\\build\\flatc-a64531bc5efc6cfc\\out" "-DCMAKE_C_FLAGS= /nologo /MD" "-DCMAKE_CXX_FLAGS= /nologo /MD"
-- Found Windows SDK v7.1: C:\Program Files\Microsoft SDKs\Windows\v7.1\
-- Configuring incomplete, errors occurred!
See also "C:/projects/flatbuffers/tests/rust_usage_test/target/debug/build/flatc-a64531bc5efc6cfc/out/build/CMakeFiles/CMakeOutput.log".
--- stderr
CMake Error at CMakeLists.txt:6 (project):
  Generator
    Visual Studio 10 2010
  given toolset specification
    host=x64
  that contains invalid field 'host=x64'.

Seems to me like it should generate cmake -G "Visual Studio 10 2010 Win64" without -Thost=x64 as pointed out in the prior issue.

rjsberry avatar Apr 05 '19 18:04 rjsberry

Thanks for the report! Can you see what version of CMake was running in that build? The relevant PR claimed that the change was supported in most recent versions of CMake

alexcrichton avatar Apr 08 '19 14:04 alexcrichton

CMake 3.13.3. I think the error is being caused by the change in this commit.

rjsberry avatar Apr 08 '19 19:04 rjsberry

Having said that the only VS version this seems to be an issue with is 10 2010.

rjsberry avatar Apr 08 '19 21:04 rjsberry

Ah ok interesting! Maybe that patch should be backed out for those versions of visual studio? Would you be up for testing a patch that does that?

alexcrichton avatar Apr 08 '19 23:04 alexcrichton

Sure, that's fine.

Do you think it's worth extending the CI for this repository to also cover Windows? Does the crate have a minimum required CMake version or minimum required VS version?

rjsberry avatar Apr 09 '19 17:04 rjsberry

Yes extending to include Windows seems fine by me, if you're interested in adding that I can switch this repository to azure pipelines so it can be more easily added.

There's no currently documented minimum CMake/VS version, it's generally "if you need it and it doesn't work we should try to add support for it, and then try to not break support for older versions"

alexcrichton avatar Apr 09 '19 19:04 alexcrichton

That sounds great. I don't have a personal Windows machine so a Windows based pipeline would be really useful for verification.

I've just taken a look at the current Travis setup and I see it's only running doc tests. Is there a particular reason no integration tests with CMake have been added? There are so many combinations of OSs and generators I'm not sure where I would start! Tier 1 platforms and unix makefiles/VS project files would be nice but... it's a pretty big job.

rjsberry avatar Apr 10 '19 17:04 rjsberry

Yes exhaustively testing this crate isn't really within the purview of the CI, I don't really personally want to maintain that and try to keep it running all the time. In general I try to just be judicious and careful about changing things, but it obviously doesn't work all the time.

I'm fine for adding some smoke tests, but I don't think there's really any need to be exhaustive on CI. It's basically impossible to be exhaustive in this case I think.

alexcrichton avatar Apr 10 '19 21:04 alexcrichton

Yeah I would agree with you there; I don't think exhausting the combinations is right, but I do think we need to (somehow) verify that the patch both works and doesn't break existing functionality.

I'd definitely like to add some tests for VS 10 2010 on i686/x86_64, but that's mainly because it's what I'm currently blocked on. I'm more than happy to add other generator targets while I'm at it, if there's any that stand out to you.

rjsberry avatar Apr 10 '19 22:04 rjsberry

Nah that sounds like a great start!

alexcrichton avatar Apr 11 '19 21:04 alexcrichton

Wasmtime building on Windows also panic at cmake 0.1.38 too:

error: failed to run custom build command for `wasmtime-wasi v0.0.0 (E:\learning-rust\wasmtime\wasmtime-wasi)`
process didn't exit successfully: `E:\learning-rust\wasmtime\target\debug\build\wasmtime-wasi-9cd4d5eba15448e8\build-script-build` (exit code: 101)
--- stderr
thread 'main' panicked at '
command did not execute successfully, got: exit code: 1

build script failed, must exit now', C:\Users\34937\.cargo\registry\src\mirrors.ustc.edu.cn-61ef6e0cd06fb9b8\cmake-0.1.38\src\lib.rs:813:5
stack backtrace:
   0: std::sys::windows::backtrace::set_frames
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\sys\windows\backtrace\mod.rs:95
   1: std::sys::windows::backtrace::unwind_backtrace
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\sys\windows\backtrace\mod.rs:82
   2: std::sys_common::backtrace::_print
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\sys_common\backtrace.rs:71
   3: std::sys_common::backtrace::print
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\sys_common\backtrace.rs:59
   4: std::panicking::default_hook::{{closure}}
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\panicking.rs:197
   5: std::panicking::default_hook
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\panicking.rs:211
   6: std::panicking::rust_panic_with_hook
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\panicking.rs:474
   7: std::panicking::continue_panic_fmt
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\panicking.rs:381
   8: std::panicking::begin_panic_fmt
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\panicking.rs:336
   9: cmake::fail
             at C:\Users\34937\.cargo\registry\src\mirrors.ustc.edu.cn-61ef6e0cd06fb9b8\cmake-0.1.38\src\lib.rs:813
  10: cmake::run
             at C:\Users\34937\.cargo\registry\src\mirrors.ustc.edu.cn-61ef6e0cd06fb9b8\cmake-0.1.38\src\lib.rs:791
  11: cmake::Config::build
             at C:\Users\34937\.cargo\registry\src\mirrors.ustc.edu.cn-61ef6e0cd06fb9b8\cmake-0.1.38\src\lib.rs:700
  12: build_script_build::main
             at .\build.rs:9
  13: std::rt::lang_start::{{closure}}<()>
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\src\libstd\rt.rs:64
  14: std::rt::lang_start_internal::{{closure}}
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\rt.rs:49
  15: std::panicking::try::do_call<closure,i32>
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\panicking.rs:293
  16: panic_unwind::__rust_maybe_catch_panic
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libpanic_unwind\lib.rs:87
  17: std::panicking::try
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\panicking.rs:272
  18: std::panic::catch_unwind
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\panic.rs:388
  19: std::rt::lang_start_internal
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\/src\libstd\rt.rs:48
  20: std::rt::lang_start<()>
             at /rustc/3de0106789468b211bcc3a25c09c0cf07119186d\src\libstd\rt.rs:64
  21: main
  22: invoke_main
             at f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:78
  23: __scrt_common_main_seh
             at f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283
  24: BaseThreadInitThunk
  25: RtlUserThreadStart

please add the Windows CI to make sure that cmake-rs is cross-platform compatible I was using 3.11.2

huangjj27 avatar Apr 21 '19 15:04 huangjj27

@alexcrichton It seems the oldest version of VS with direct support via Azure pipelines is 2015 via the vs2015-win2012r2 image.

The CI job for 2010 might be possible by using our own Dockerfile launched in the win1803 image, but I've never actually tried to install VS build tools in a Docker image, and after a quick search the outlook for doing that with such an old version doesn't look great.

rjsberry avatar Apr 25 '19 21:04 rjsberry

We can do the best we can to add CI, but I wouldn't want to bend over backwards too much to test every version of CMake/VS that we can

alexcrichton avatar Apr 26 '19 14:04 alexcrichton

No. But it seems a shame I can't easily test the target version of VS that I'm having an issue with!

rjsberry avatar Apr 26 '19 16:04 rjsberry