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

Add pending_interpreted #14386

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Mar 22, 2024

This is similar to #14086, and additionally it provides pending_interpreted!, since in some cases it is easier to skip the affected specs from the spec helpers they use, rather than in the specs themselves.

The remaining files are:

  • llvm/*_spec.cr: The shared library loader is unable to locate libstdc++.so. On my system the only file with that exact name is /usr/lib/gcc/x86_64-linux-gnu/13/libstdc++.so, which is not a default search directory; instead libstdc++.so.6 can be found in other directories. It looks like LibLLVM doesn't actually need @[Link("stdc++")], otherwise it would have been covered by llvm-config --system-libs? Still, this implies a discrepancy between our loader and the actual ld. All the specs will pass if we remove that annotation.
  • syscall_spec.cr: I don't know if this is ever planned for interpreted code. The only place in the standard library that uses Syscall has an interpreter-specific patch.
  • time/time_spec.cr, time/format_spec.cr: They have been broken for a while, but Fix requires for time/time_spec.cr and time/format_spec.cr #14385 fixes them.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Very nice!

require "signal"

describe "Signal" do
# interpreted code never receives signals (#12241)
pending_interpreted describe: "Signal" do
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Wouldn't it make more sense to skip this entire file like on wasm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

skip_file suggests signals won't be on WebAssembly at all, but it looks like there are multiple attempts at POSIX compatibility layers, so I think skip_file is the less appropriate one here (or rather, it hasn't been updated to use pending_wasm32 after #14086)

Copy link
Member

Choose a reason for hiding this comment

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

Could be.

My main concern is that I find pending_interpreted describe: "" over the entire contents of a file very unergonomic. It prevents grepping describe Signal, for example. And it's needles complexity.

I'd prefer to skip the file for wasm and interpreter and add comments to explain this might be enabled in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are already two uses of pending_wasm32 describe:, so IMO this should be taken care of in a separate PR.

@straight-shoota straight-shoota added this to the 1.12.0 milestone Mar 22, 2024
@straight-shoota straight-shoota merged commit 905039c into crystal-lang:master Mar 24, 2024
58 checks passed
@HertzDevil HertzDevil deleted the feature/pending-interpreted branch March 26, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants