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

Enable the interpreter on Windows #14964

Merged

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
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
@@ -266,13 +267,47 @@ jobs:
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
64 of 66 checks passed
@HertzDevil HertzDevil deleted the feature/windows-interpreter branch September 7, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Outstanding issues for the interpreter on Windows
2 participants