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 TruffleRuby in CI and skip the few specs which are not yet supported on TruffleRuby #755

Closed
wants to merge 10 commits into from

Conversation

eregon
Copy link
Collaborator

@eregon eregon commented Mar 25, 2020

Also adding a few extra specs and fixing the long double spec to actually run.
See the individual commit messages for details.

Part of #660 (comment)

@eregon
Copy link
Collaborator Author

eregon commented Mar 25, 2020

long double specs seem to segfault on Windows, should we simply exclude them?
https://ci.appveyor.com/project/larskanis/ffi-aofqa/builds/31713641/job/p7uuncjd3t73838n

@eregon
Copy link
Collaborator Author

eregon commented Mar 25, 2020

I can split the long double change in another PR if you prefer (it's just a rename currently)

* This is generally handled in FFI::Pointer #write/#put methods.
* Happens when running spec/ffi/long_double_spec.rb:
  warning: BigDecimal.new is deprecated; use BigDecimal() method instead.
@eregon eregon force-pushed the specs-passing-on-truffleruby branch from bb33ee8 to a3e1b2a Compare March 25, 2020 16:21
spec/ffi/long_double_spec.rb Show resolved Hide resolved
spec/ffi/struct_spec.rb Show resolved Hide resolved
@larskanis
Copy link
Member

LGTM, I'll merge this. It's probably better to separate the coerce test from the other struct tests. I'll open a PR for review.

@eregon
Copy link
Collaborator Author

eregon commented Mar 28, 2020

I'll rebase this to get the CI fixes.

There are still a few issues preventing the CI to pass on TruffleRuby, I'll keep working on it: https://travis-ci.org/github/ffi/ffi/builds/667891953

@eregon
Copy link
Collaborator Author

eregon commented Mar 28, 2020

@larskanis I see you merged it manually to master, thanks!
(except 1f6618f, which doesn't matter)

@eregon eregon closed this Mar 28, 2020
@larskanis
Copy link
Member

Great work! I added f2680e6 instead of 1f6618f . Could you please have a look at the remaining Truffleruby issues here?

@@ -41,7 +41,9 @@ rbffi_num2longdouble(VALUE value)

if (RTEST(rb_cBigDecimal) && rb_cBigDecimal != rb_cObject && RTEST(rb_obj_is_kind_of(value, rb_cBigDecimal))) {
VALUE s = rb_funcall(value, rb_intern("to_s"), 1, rb_str_new2("E"));
return strtold(RSTRING_PTR(s), NULL);
long double ret = strtold(RSTRING_PTR(s), NULL);
RB_GC_GUARD(s);
Copy link
Member

Choose a reason for hiding this comment

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

Is this required in Truffleruby? In MRI there is no need for a guard here IMHO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No it's not, TruffleRuby doesn't use the C extension for FFI.
I did it to try to debug this long double failure on Windows.
Why is it not needed on MRI, couldn't s be GC'd during the strtold call?

@eregon
Copy link
Collaborator Author

eregon commented Mar 29, 2020

Could you please have a look at the remaining Truffleruby issues here?

On macOS, it fails because TravisCI uses a too old version on macOS (10.13):
https://travis-ci.org/github/ffi/ffi/jobs/668207490
Moving those jobs to GitHub Actions would solve that (relates to #760), any thought on that? We could start moving the ones that are easy to move (MRI + TruffleRuby on Linux/macOS/could also move Windows), and leave the rest on TravisCI, that would speed up CI and test the main platforms on a more stable CI system.

On Linux I need to add the new LONG_DOUBLE_SIZE constant in TruffleRuby, since it was added in the C extension.
For the benchmark failure I'll need to investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants