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

Suggestion: Windows CI #75

Closed
rjsberry opened this issue Apr 5, 2019 · 14 comments · Fixed by #158
Closed

Suggestion: Windows CI #75

rjsberry opened this issue Apr 5, 2019 · 14 comments · Fixed by #158

Comments

@rjsberry
Copy link

rjsberry commented Apr 5, 2019

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.

@alexcrichton
Copy link
Member

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

@rjsberry
Copy link
Author

rjsberry commented Apr 8, 2019

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

@rjsberry
Copy link
Author

rjsberry commented Apr 8, 2019

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

@alexcrichton
Copy link
Member

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?

@rjsberry
Copy link
Author

rjsberry commented Apr 9, 2019

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?

@alexcrichton
Copy link
Member

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"

@rjsberry
Copy link
Author

rjsberry commented Apr 10, 2019

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.

@alexcrichton
Copy link
Member

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.

@rjsberry
Copy link
Author

rjsberry commented Apr 10, 2019

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.

@alexcrichton
Copy link
Member

Nah that sounds like a great start!

@huangjj27
Copy link

huangjj27 commented Apr 21, 2019

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

@rjsberry
Copy link
Author

@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.

@alexcrichton
Copy link
Member

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

@rjsberry
Copy link
Author

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

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 a pull request may close this issue.

3 participants