corrosion icon indicating copy to clipboard operation
corrosion copied to clipboard

Fix windows-latest CI issues

Open jschwe opened this issue 2 years ago • 5 comments

  • Replace undocumented cmake -H option with cmake -S, since -H is aliased with --help on more recent cmake versions.
  • Use the default shell in CI where possible, which means powershell on windows. This is required because git bash does not work with the msvc-dev-cmd action we need for msvc.
  • Use Yaml > syntax to replace line continuations with spaces for long commands. Powershell line continuation is space + backtick, so we can't just use \.

jschwe avatar Feb 25 '22 10:02 jschwe

Thanks for looking into that. I really didn't want to investigate what needed to be fixed there and this is why i pinned the previous version.

ogoffart avatar Feb 25 '22 14:02 ogoffart

@ogoffart Could you fork https://github.com/AndrewGaspar/install-cmake into the corrosion-rs organization? I need to update the action, since the artifact paths for cmake have changed for more recent versions of cmake. To test windows-2022 with MSVC we need at least Cmake 3.21, and we can't install that without updating the action.

Regarding this pull-request - The scope has somewhat changed - Since upgrading to windows-2022 raises the minimum supported cmake version for windows, this change does not make much sense by itself.

I would just test that windows-2022 works in general and in the same scope try to reduce the amount of combinations we are testing in the matrix.

jschwe avatar Feb 25 '22 18:02 jschwe

Okay, this PR is kind of stuck now, since I can't reproduce why cross-compiling with windows and msvc is failing in CI.

Observations:

  • On both windows-2022 and windows-2019 with the msvc toolchain, the cross-compiling test fails if Rust < 1.54. Judging from the logs it seems as if a build script is erroneously linked with cross-compiled libraries.
  • I can't reproduce the failures on my local machine with windows 11 and msvc 2022 17.1 (and powershell 5/7).

So currently going forward with this PR does not make much sense, unless we would want to increase the MSRV anyway.

jschwe avatar Mar 14 '22 19:03 jschwe

Yes, we can increase the msrv for newer msvc. If someone can increase the msvc version, they certainly can upgrade rustc

ogoffart avatar Mar 14 '22 20:03 ogoffart

Okay, I more or less figured out what is going on here.

  1. I was not able to reproduce because I was using PowerShell and running vsvarsall.bat has no effect there, because it is meant for the windows command prompt and not powershell. After figuring that out and setting the required env variables with the help of StackOverflow , I could reproduce the linking issue locally. This would have been much faster if I had better knowledge about shells on windows :D.

  2. This is not really a corrosion or powershell issue, but a cargo issue (that presumably got fixed in 1.54). Basically what happens is that vcvarsall.bat sets environment variable (e.g. VSCMD_ARG_HOST_ARCH and VSCMD_ARG_TGT_ARCH). Before 1.54 this has the effect, that a build script build which invokes the linker would fail, since the build script msvc linker would assume (based on the environment variables) that it should cross-link. Of course the build script should be compiled and linked for the host platform, so linking fails.

This can be reproduced without corrosion with a minimal example:

Cargo.toml:

[package]
name = "vs_cc_bs_test"
version = "0.1.0"
edition = "2018"

[dependencies]

[build-dependencies]
proc-macro2 = "1.0.36"

build.rs:

// Add dependency
#[allow(unused_imports)]
use proc_macro2;


fn main() {
    for var in std::env::vars() {
        println!("{:?}", var);
    }
}

If you then setup the environment correctly for msvc cross-compiling via vsvarsXXX.bat and set CC, CXX to cl, and then run cargo +1.45 build --target=aarch64-pc-windows-msvc, linking of the build script will fail. With more recent versions of cargo this will not fail anymore.

  1. As to why our cross-compile tests work with git bash: If you use git bash, then I guess the environment is not "properly initialized", so msvc does not know that it should cross-compile. I'm not sure what further implications this has on what is actually compiled and linked in this case.

So to summarize:

Due to an issue in cargo before 1.54, cross-compiling with msvc may not work if you have a build script. If you happen to have cl in path already, and don`t setup the msvc environment variables, the it may work, but I'm not sure what other implications this has, since msvc may not work correctly if the environment is not correctly initialized.

So for corrosion this should probably mean that we:

  • Increase the official MSRV for windows-msvc to 1.54 if cross-compilation is selected.

If someone can increase the msvc version, they certainly can upgrade rustc

For clarification, this is not related to the MSVC version and seems to only be related to cargo. The issue came to light, because cl was not in path anymore with git bash on windows-2022, which caused me to use powershell, where the environment is correctly initialized for VS, which made the problem appear.

jschwe avatar Mar 17 '22 18:03 jschwe