-
Notifications
You must be signed in to change notification settings - Fork 61
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
[163] Integrate with rb-sys
#172
base: master
Are you sure you want to change the base?
Conversation
806e0d6
to
0da4a3f
Compare
rb-sys
& some minor changes (in place version)rb-sys
& some minor changes (in place version)
Outdated comment which was fixed (setup issue)Doesn't seem to work at all on Windows. ![image](https://user-images.githubusercontent.com/7335788/224157746-4a92c597-e8ee-48f5-8303-c89271bfa28b.png)Also, I decided to use rutie even though someone said magnus is better because magnus simply doesn't build with the target
|
Hey @NuriYuri, I'm more than happy to support |
Hi, thanks for the feedback :) Fortunately I have the machine for those tests so opened an issue on rb-sys as advised :) |
Since I can now build rb-sys I got the following issue with the code in your branch (type missmatch) Logs (click to see)Compiling rb-sys v0.9.71 error[E0308]: mismatched types --> src\rubysys\array.rs:92:16 | 92 | if flags & (RArrayEmbed::Flag as u64) == 0 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u32`, found `u64` Branch I built: |
@NuriYuri Could you use the Github "spoiler" feature to hide your long console output from your earliest post? Here's an example on how: https://gist.github.com/jbsulli/03df3cdce94ee97937ebda0ffef28287#how-to |
5f422a9
to
1fd2d5b
Compare
@goyox86 you may want to explore using |
Oh! Going to check that out I was actually debugging segfaults today because of VM::init and thread safety in tests. Thanks for the heads up @ianks ! |
41e1fec
to
d704d17
Compare
rb-sys
& some minor changes (in place version)rb-sys
479b590
to
9184716
Compare
@ianks @danielpclark I think we are in a good position for another review pass. We are green on 2.7, 3.0. 3.1, 3.2 and HEAD on Linux, macOS, Windows. ❕ ❕ |
41e7625
to
2bb3103
Compare
46ba5f3
to
fa2d0e1
Compare
}; | ||
|
||
use rb_sys::{RString, VALUE}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianks what are the warnings for RString suggesting we do for rb_sys?
warning: use of deprecated struct
rb_sys::RString
: To improve API stability with ruby-head, direct usage of Ruby internal structs has been deprecated. To migrate, please replace the usage of this internal struct with its counterpart in therb_sys::stable
module. For example, instead ofuse rb_sys::rb_sys__Opaque__ExampleStruct;
, useuse rb_sys::stable::ExampleStruct;
. If you need to access the internals of these items, you can use the providedrb-sys::macros
instead.
src/class/traits/object.rs
Outdated
@@ -308,7 +301,7 @@ pub trait Object: From<Value> { | |||
/// let mut string = RString::new_utf8("Some string"); | |||
/// | |||
/// // The same can be done by modifying `string.singleton_class()` | |||
/// // or using `string.define_singleton_method("greeting", greeting)` | |||
/// // or usiang `string.define_singleton_method("greeting", greeting)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goyox86 Typo added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 0a743fb
src/binding/class.rs
Outdated
} | ||
|
||
pub fn define_method<I: Object, O: Object>(klass: Value, name: &str, callback: Callback<I, O>) { | ||
let name = util::str_to_cstring(name); | ||
|
||
unsafe { | ||
class::rb_define_method(klass, name.as_ptr(), callback as CallbackPtr, -1); | ||
let callback = callback as *const libc::c_void; | ||
class::rb_define_method(klass, name.as_ptr(), callback, -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goyox86 I know this is a minor nitpick. But is there a reason why the three instances of callback as CallbackPtr
were changed here in this file and once in src/binding/module.rs
? If these don't need to be changed I think they should remain as they were.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right! addressed in ae5896c
@@ -110,6 +117,9 @@ pub fn wrap_data<T>(klass: Value, data: T, wrapper: &dyn DataTypeWrapper<T>) -> | |||
unsafe { typed_data::rb_data_typed_object_wrap(klass, data, wrapper.data_type()) } | |||
} | |||
|
|||
// TODO: Skipped the lint, but this function takes an immutable reference and returns a mutable | |||
// one. Changing the signature is a breaking change. What do we do? | |||
#[allow(clippy::mut_from_ref)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goyox86 For breaking changes like this you should add a section to the README of how to migrate from Rutie 0.9 to 0.10 where Ruby 3 works and include notes on breaking changes. The support for Ruby 3 should be Rutie release 0.10.0.
NOTE: Any signature changes for methods that are usable outside the crate should have breaking changes notes in the upgrade notice added to the README.md
.
} else { | ||
// After 3.1 we get TRUE/true | ||
bool_to_value(c_int_to_bool(result)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a test added for this as ruby version specifics have been added to it? If it changed now then perhaps it changes in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment // After 3.1 we get TRUE/true
says nothing of false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sure if this is what you meant @danielpclark but I added a small test in here to detect if because of changes in the implementation of rb_eql
things break at this level. The CI runs for all these ruby versions so we should be able to pin point which version is the offender.
pub fn econv_prepare_opts(opthash: Value, opts: *const Value) -> c_int { | ||
unsafe { encoding::rb_econv_prepare_opts(opthash, opts) } | ||
pub fn econv_prepare_opts(opthash: Value, opts: *mut Value) -> c_int { | ||
unsafe { encoding::rb_econv_prepare_opts(opthash, opts as *mut _) } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about various changes throughout the code where we're going from *const
to *mut
. Perhaps a simple explanation on why Value
can be *mut
here would suffice. Just a reply here should be fine.
- os: windows-latest | ||
ruby_static: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience with past Ruby version managers the Mac OS would by default install a statically linked built Ruby but Rutie ran best with dynamic linking. The CI test suite accounted for that and permitted failures on the static build. There were 3 failures that I remember for it.
What does the new CI test suite do for the differences between static and dynamic linking of Ruby? @goyox86
// | ||
// let num = str_to_num("0").unwrap(); | ||
// assert_eq!(::std::u32::MIN, num.to_u32()); | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an issue it should probably be opened as a task under Issues.
@@ -100,7 +96,7 @@ extern "C" { | |||
} | |||
|
|||
pub unsafe fn coderange_set(obj: Value, code_range: InternalValue) { | |||
let basic: *mut RBasic = mem::transmute(obj.value); | |||
let basic: *mut RBasic = obj.value as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what this change does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an equivalent way of achieving the same result, as
is safer than transmute since transmute
can also change types of non-pointer types and also lifetimes. In general wherever you can get away with an as
instead of a transmute we should probably should stick to as
.
src/util.rs
Outdated
/// Class::from_existing("String").define_method("==", string_eq); | ||
/// } | ||
/// ``` | ||
/// TODO: Made this public function unsafe due to clippy linting, should we just skip the lint? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just mark the breaking change in the README. This note can then be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 0a743fb
Amazing work @goyox86 ! I'm excited for this. We're almost there. |
@goyox86 Once my current comments are addressed I would be ready to merge this in to the main branch and release a new gem release with the changes. As to the question on the description as to whether the commits should be squashed - I don't believe there is a need for that. |
Hey @danielpclark! Great news I will make some time this week to address the comments 🙏 |
95673d7
to
6143159
Compare
… now. Let's wait until 3.4 comes out. Also added libruby static for windows on CI.
@goyox86 Where are we at on this? I'm expecting a thorough documentation of breaking changes notes in the README. I don't know though if you were expecting to do that. With all the changes it's not likely the one method is the only breaking change. What are the changes to the process or usage? Once I'm relatively certain we're informing people of what they're in for with the upgrade I'll move forward with this. |
Also looking at the test suite. I believe you added a config for static so that we have both dynamic and statically linked Rubies tested. But I'm not seeing the result of that. |
Happy to give another review once ready as well, since it has been awhile since the first one |
Addresses #163
This PR migrates using the
rb-sys
crate to interface with CRuby instead of the homegrown derived from Rubysys.Additionally it incorporates improvements to the Rust crate and migrates the CI to Github Actions along some
refreshments to Rust crate code (Rust 2021).
Highlights
rb_sys
we now support Ruby2.7
,3.0
,3.1
,3.2
andmaster
on Linux, MacOS, and Windows and master all integrated now in CI.rb_sys
infrastructure to setup our build environment.rb_sys
instead of defining them ourselves.From
instead ofInto
to get theInto
impl for free).rake
version 12.rb_sys
'sruby_test
macro we don't need to lock globals on tests, and there are no weird crashes in MacOS and Windows.rb_sys
facilities where possible.Fixnum
s around and we don't have those anymore, so I replaced byInteger
.I think there are two breaking changes, I marked them as To-dos, they are most around marking stuff as
unsafe
.Left to do
rb_sys
that allows me to use the new cargo features, since I am using head from Git now. @ianks WDYT?libruby
linking see if the new version ofrutie
would be backwards compatible.