CI: test windows

[lucascolley]

No description provided.

[lucascolley]

@steppi it looks like we are hitting build errors with MSVC.

[steppi]

The problem is with the xsref path set in an environment variable here

xsf/.github/workflows/tests.yml

Line 20 in 55ac1f0

XSREF_TABLES_PATH: "${{ github.workspace }}/xsref/tables"

It nees to be a valid windows path but it's hardcoded with / path separators. Maybe just branch in the workflow to set the path correctly based on platform.

[lucascolley]

XSREF_TABLES_PATH: D:\a\xsf\xsf\xsref\tables

Okay, that looks fixed, but there are still lots of errors

[steppi]

On a Windows VM. I can't even get to the build step locally. MSVC isn't being found.

Pixi task (configure-tests in default): cmake -DBUILD_TESTS=ON "-S ." "-B build"
CMake Error at CMakeLists.txt:2 (project):
  Generator

    Visual Studio 16 2019

  could not find any instance of Visual Studio.

It looks like the Github actions Windows runners ship with a lot of stuff that isn't on a fresh Windows install https://github.com/actions/runner-images/blob/main/images/windows/Windows2022-Readme.md,

Do you have a Windows machine to test things out on @lucascolley? Could it be something to do with me having Visual Studio 2022 installed, but this is expecting 2019?

[lucascolley]

I see the exact same:

Pixi task (configure in default): cmake "-S ." "-B build"
-- Building for: Visual Studio 16 2019
CMake Error at CMakeLists.txt:2 (project):
  Generator

    Visual Studio 16 2019

  could not find any instance of Visual Studio.

I guess we need to add a dependency for windows. I'll do some digging

[steppi]

In the meantime, explicitly making the configure command

cmake -G "Visual Studio 17 2022" -DBUILD_TESTS=ON "-S ." "-B build"

worked for me. I can debug the errors we're seeing in CI at least. This is all running inside of a Developer PowerShell for VS 2022.

[steppi]

This still fails locally for me, but the XSREF_TABLES_PATH issue has been fixed. The previous attempt at a fix didn't work because the environment variable was being taken from pixi.toml, not the workflow file. What I've done here is force the path to be the xsref repo that pixi clones into xsf, and hardcode that into the CMakeLists.txt, where path separators are not platform dependent since cmake normalizes them.

The reason it still fails is because MSVC requires all linked libraries to be built with the same mode, but the tests here are built in debug mode, but the conda feedstock for catch2 builds in release mode. See https://github.com/conda-forge/catch2-feedstock/blob/a01d2f0df80cc2334b06839f98526f41e4377fb8/recipe/build.sh#L17.

Some possibilities:

1. Go back to building Catch2 ourselves after cloning the repo.
2. Ask the feedstock maintainer to offer both release and debug builds of catch2.
3. Build the tests in release mode? But that seems kind of silly.

Example error message:

Catch2.lib(catch_registry_hub.cpp.obj) : error LNK2038: mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '0' doesn't match value '2' in test_bdtri.obj [D:\a\xsf\xsf\build\tests\scipy_special_tests_test_bdtri.vcxproj] Catch2.lib(catch_registry_hub.cpp.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MDd_DynamicDebug' in test_bdtri.obj [D:\a\xsf\xsf\build\tests\scipy_special_tests_test_bdtri.vcxproj]

relevant stackoverflow post:

https://stackoverflow.com/questions/7668200/error-lnk2038-mismatch-detected-for-iterator-debug-level-value-0-doesnt

[lucascolley]

the environment variable was being taken from pixi.toml, not the workflow file.

hmm, that shouldn't be the case... https://pixi.sh/latest/workspace/advanced_tasks/#environment-variables

[steppi]

the environment variable was being taken from pixi.toml, not the workflow file.

hmm, that shouldn't be the case... https://pixi.sh/latest/workspace/advanced_tasks/#environment-variables

Sorry. I accidentally edited your comment instead of quote replying it. Fixed.

I deleted this line from pixi.toml

configure-tests.env.XSREF_TABLES_PATH = "$PWD/xsref/tables"

after discovering with some static asserts that this is what was getting used for XSREF_TABLES_PATH, not the one defined in the workflows. I'm not sure how or why, but it's what I observed. It's not an issue anymore though, because I sidestepped it. I think it may be some windows specific thing since environment variables work differently there.

[lucascolley]

okay, thanks, sounds like a bug. I'll try to craft a reproducer, because that should have worked

[lucascolley]

Ask the feedstock maintainer to offer both release and debug builds of catch2.

That sounds like the best long-term solution

[lucascolley]

No luck reproducing locally. If I had to guess, perhaps variables in $GITHUB_ENV aren't propagated between steps until you request them explicitly with $....

[steppi]

That sounds like the best long-term solution

I just realized that we might want to test in Release mode, since sometimes aggressive compiler optimizations cause numerical issues even if --ffastmath isn't enabled, and it's good to be aware of when that happens.

[steppi]

OK, after building in Release mode, we're left with only the weird errors that I don't understand yet.

@lucascolley, I know you wanted a one liner, but I think it would be helpful if there were separate build and test steps in the jobs so it's easy to see at a glance if the build failed or if the tests failed.

[steppi]

Ha. There were some instances where I used and instead of && in code that I translated from Cython. That was still working due to a nonstandard extension that isn't available in MSVC. That should take care of the build errors. Now it's only test failures due to no having Windows tolerance files yet.

[lucascolley]

I think it would be helpful if there were separate build and test steps in the jobs so it's easy to see at a glance if the build failed or if the tests failed.

done 👍

[steppi]

The windows failures are all legitimate failures now. They involve returning complex number with NaNs instead of some particular direction of infinity. If we had MSVC specific tolerance files for these, the accepted tolerance would be inf. It would probably be good to get these to actually pass though.

It's not as straightforward as it was for Mac and Linux currently to generate an MSVC specific tolerance file because we use SciPy to generate the initial tolerance files, and I am unable to build a usable SciPy with MSVC. The reason we used SciPy and not xsf itself is to protect against regressions that may have occured in the xsf codebase after it diverged from SciPy. Doing this actually helped me find a couple commits which were not cherry-picked from SciPy into xsf.

Sometime in the next few weeks, I plan to write workflows in the xsref repo which run the test cases in xsf using xsf to generate updated tolerance files. After that, it will be easier to create as many CI job specific tolerance files as we need.

I'm fine with merging this as is and iterating from here. We're just getting started, so I think it's fine if we have some known failures in the test suite. I'd consider both the complex infinity handling and additional work needed for platform specific tolerance file generation asout of scope for this PR. I'll follow up in the next few weeks, but in the meantime I have other priorities.

[lucascolley]

Thanks Albert, nice work!

I am unable to build a usable SciPy with MSVC

I have also been trying (and failing) to do this, progress update at rgommers/pixi-dev-scipystack#23 (comment)