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

Remove spec/win32_std_spec.cr #12282

Merged

Conversation

HertzDevil
Copy link
Contributor

With just 8 out of 259 spec files failing to compile on Windows, there is no longer a dire need to keep a list of the spec files that do build. This PR makes the remaining specs use skip_file instead, similar to the Unix domain socket ones. It looks like this enables 3 new examples somewhere.

The script that generates spec/win32_std_spec.cr remains intact in case we want to reuse it for some other completely new system.

@HertzDevil HertzDevil added platform:windows Windows support based on the MSVC toolchain / Win32 API kind:specs labels Jul 16, 2022
Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @HertzDevil 🙏

@straight-shoota straight-shoota added this to the 1.6.0 milestone Jul 17, 2022
@straight-shoota
Copy link
Member

straight-shoota commented Jul 19, 2022

👀

1) HTTP::Client tests write_timeout
     Failure/Error: expect_raises(IO::TimeoutError, {% if flag?(:win32) %} "WSASend timed out" {% else %} "Write timed out" {% end %}) do
       Expected IO::TimeoutError, got #<Socket::ConnectError: Error connecting to 'localhost:64862': Overlapped I/O event is not in a signaled state.> with backtrace:
         # src\socket\addrinfo.cr:67 in 'initialize'
         # src\socket\tcp_socket.cr:27 in 'initialize'
         # src\socket\tcp_socket.cr:27 in 'new'
         # src\http\client.cr:789 in 'io'
         # src\http\client.cr:671 in 'send_request'
         # src\http\client.cr:603 in 'exec_internal_single'
         # src\http\client.cr:596 in 'exec_internal'
         # src\http\client.cr:581 in 'exec'
         # src\http\client.cr:713 in 'exec'
         # src\http\client.cr:406 in 'post:body'
         # spec\std\http\client\client_spec.cr:296 in '->'
         # src\spec\example.cr:45 in 'internal_run'
         # src\spec\example.cr:33 in 'run'
         # src\spec\context.cr:18 in 'internal_run'
         # src\spec\context.cr:339 in 'run'
         # src\spec\context.cr:18 in 'internal_run'
         # src\spec\context.cr:156 in 'run'
         # src\spec\dsl.cr:220 in '->'
         # src\crystal\at_exit_handlers.cr:14 in 'run'
         # src\crystal\main.cr:50 in 'exit'
         # src\crystal\main.cr:45 in 'main'
         # src\crystal\main.cr:127 in 'main'
         # src\crystal\system\win32\wmain.cr:35 in 'wmain'
         # D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288 in '__scrt_common_main_seh'
         # C:\Windows\System32\KERNEL32.DLL +85712 in 'BaseThreadInitThunk'
         # C:\Windows\SYSTEM32\ntdll.dll +517019 in 'RtlUserThreadStart'

This failing spec is already activated before this PR, so it's probably not a new addition.

@HertzDevil
Copy link
Contributor Author

I have been getting that error intermittently on local builds as well, plus this one from the first CI run:

Unhandled exception in spawn: WSASend: An established connection was aborted by the software in your host machine. (IO::Error)
  from src\io\overlapped.cr:181 in 'unbuffered_write'
  from src\io\buffered.cr:136 in 'write'
  from src\io.cr:877 in 'write_byte'
  from src\io\buffered.cr:168 in 'write_byte'
  from src\char.cr:897 in 'to_s'
  from src\io.cr:174 in '<<'
  from src\http\client\response.cr:72 in 'to_io'
  from spec\std\http\client\client_spec.cr:17 in '->'
  from src\fiber.cr:146 in 'run'
  from src\fiber.cr:98 in '->'

This blocks the whole spec. I have no idea if #12281 caused these errors or the root cause is simply an incomplete event loop implementation. I got one CI run green.

@straight-shoota
Copy link
Member

This looks very much like a flaw in the event loop.

@straight-shoota straight-shoota removed this from the 1.6.0 milestone Jul 25, 2022
@straight-shoota straight-shoota added the pr:needs-work A PR requires modifications by the author. label Jul 25, 2022
@HertzDevil
Copy link
Contributor Author

The three new examples introduced in this PR are spec/std/crystal/compiler_rt/{ashlti3,ashrti3,lshrti3}_spec.cr. It is rather unlikely they would break the concurrency-related specs.

I am wondering if the failures can also be reproduced by simply rearranging the requires in win32_std_spec.cr.

@straight-shoota straight-shoota removed the pr:needs-work A PR requires modifications by the author. label Sep 29, 2022
@straight-shoota straight-shoota added this to the 1.6.0 milestone Sep 29, 2022
@straight-shoota
Copy link
Member

CI is green consistently, so I think this should be good to merge.
If the failure shows up again, it's probably not related to this change.

@straight-shoota straight-shoota merged commit d72dd15 into crystal-lang:master Sep 30, 2022
@HertzDevil HertzDevil deleted the spec/remove-win32_std_spec branch September 30, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:specs platform:windows Windows support based on the MSVC toolchain / Win32 API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants