Skip to content

Enable the interpreter on Windows#14964

Merged
straight-shoota merged 10 commits intocrystal-lang:masterfrom
HertzDevil:feature/windows-interpreter
Sep 7, 2024
Merged

Enable the interpreter on Windows#14964
straight-shoota merged 10 commits intocrystal-lang:masterfrom
HertzDevil:feature/windows-interpreter

Conversation

@HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Sep 4, 2024

Closes #12396.

A good chunk of networking specs are disabled because they call Crystal::System::Socket.accept_ex.call, which is not yet supported in the interpreter; this is now tracked in #12495. There is also a workaround for the event loop issue mentioned in #14949 (comment), and another for the lack of Reference.pre_initialize support (this won't be necessary after #14968).

On the other hand, Process indeed works, so interpreter support on Windows is not strictly inferior to Unix-like systems.

@HertzDevil HertzDevil added kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:compiler:interpreter labels Sep 4, 2024
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Sep 4, 2024

It looks like CI fails because it is not linking the correct DLLs:

C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.422-5\x64\bin\api-ms-win-crt-conio-l1-1-0.dll
C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.422-5\x64\bin\api-ms-win-crt-process-l1-1-0.dll

On my system C:\WINDOWS\System32\ucrtbase.dll is linked twice in place of these entries. I now managed to SSH into a GitHub Actions runner using mxschmitt/action-tmate, and this is what I got:

>where api-ms-win-crt-convert-l1-1-0.dll
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\api-ms-win-crt-convert-l1-1-0.dll
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\Tools\api-ms-win-crt-convert-l1-1-0.dll
C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.422-5\x64\bin\api-ms-win-crt-convert-l1-1-0.dll
C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit\api-ms-win-crt-convert-l1-1-0.dll
C:\Program Files\LLVM\bin\api-ms-win-crt-convert-l1-1-0.dll
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\Llvm\x64\bin\api-ms-win-crt-convert-l1-1-0.dll

>where api-ms-win-crt-conio-l1-1-0.dll
C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.422-5\x64\bin\api-ms-win-crt-conio-l1-1-0.dll
C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit\api-ms-win-crt-conio-l1-1-0.dll
C:\Program Files\LLVM\bin\api-ms-win-crt-conio-l1-1-0.dll
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\Llvm\x64\bin\api-ms-win-crt-conio-l1-1-0.dll

The same thing on my machine gave:

>where api-ms-win-crt-convert-l1-1-0.dll
C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\api-ms-win-crt-convert-l1-1-0.dll
C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\Tools\api-ms-win-crt-convert-l1-1-0.dll

>where api-ms-win-crt-conio-l1-1-0.dll
INFO: Could not find files for the given pattern(s).

It appears Crystal::Loader is picking up whatever it finds in %PATH%. However this page caught my attention:

On Windows 10 and Windows 11, the Universal CRT in the system directory is always used, even if an application includes an application-local copy of the Universal CRT. It's true even when the local copy is newer, because the Universal CRT is a core operating system component on Windows 10 and later.

This suggests API sets should completely bypass path searching and use LoadLibrary directly?

@HertzDevil HertzDevil marked this pull request as ready for review September 4, 2024 20:43
run: make -f Makefile.win samples

x86_64-windows-release:
if: github.repository_owner == 'crystal-lang' && (startsWith(github.ref, 'refs/tags/') || startsWith(github.ref, 'refs/heads/ci/'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: This change means building a compiler in release mode on every workflow run. I'm wondering if it makes sense to keep the non-release build around then? We could build just a single compiler (in release mode) and use that for x86_64-windows-test as well.

Maybe we should do this as a follow-up though.

@straight-shoota straight-shoota added this to the 1.14.0 milestone Sep 4, 2024
@HertzDevil
Copy link
Contributor Author

For the record, using a non-release build of the interpreter takes almost 6 hours to finish the stdlib specs

@straight-shoota straight-shoota merged commit cdd9ccf into crystal-lang:master Sep 7, 2024
@HertzDevil HertzDevil deleted the feature/windows-interpreter branch September 7, 2024 12:07
straight-shoota pushed a commit that referenced this pull request Sep 16, 2024
Since  #14964 we're building the compiler in release mode on every workflow run. We can use that build instead of the non-release build for workflow jobs that need a compiler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:compiler:interpreter

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Outstanding issues for the interpreter on Windows

2 participants