[WIP] Ruby bindings #67

kastiglione · 2016-09-18T05:28:55Z

⚠️ Not Ready ⚠️

This adds a basic Ruby extension. The initial direction on this is to make a superficial Ruby API that does minimally more than wrapping the C API.

TODO

Just the whole API and everything
Update Makefile (and maybe CMakeLists.txt)
Add error handling, RData type checking
Figure out infrequent rb_gc_mark crash
Write docs (YARD)
Unit tests?
Integration tests?

kastiglione · 2016-09-18T05:30:55Z

At this point we have:

p = Arbiter::ProjectIdentifier.new('yolo')
r = Arbiter::Requirement.at_least(Arbiter::SemanticVersion.new(1, 2, 3))
d = p.create_dependency(r)

kastiglione · 2016-09-18T05:33:25Z

bindings/ruby/arbiter.c

+}
+
+static void project_identifier_mark(ArbiterUserValue *user_value) {
+  rb_gc_mark((VALUE)user_value->data);


In my irb sessions, there's an infrequent crash happening here. Perhaps data needs to be nilled out in the free function passed to Data_Wrap_Struct.

The problem was the parameter type, it should be ArbiterProjectIdentifier * 🙈. Someone should create a TypedC.

kastiglione · 2016-09-18T05:36:48Z

bindings/ruby/arbiter.c

+
+static VALUE requirement_compatible_with(VALUE klass, VALUE version) {
+  ArbiterSemanticVersion *semantic_version = DATA_PTR(version);
+  ArbiterRequirement *requirement = ArbiterCreateRequirementCompatibleWith(semantic_version, ArbiterRequirementStrictnessAllowVersionZeroPatches);


Ruby doesn't need no ArbiterRequirementStrictnessStrict, right?

jspahrsummers · 2016-09-18T22:16:43Z

bindings/ruby/arbiter.c

+    .lessThan = value_less_than,
+    .hash = value_hash,
+    .createDescription = value_create_description,
+    .destructor = NULL,


Hmm… this seems wrong, and might relate to the crashes you're seeing. Although Ruby is GCed, you're providing data to Arbiter here, so you need to effectively add a new GC root while Arbiter has a reference to it, and then remove it once Arbiter invokes your destructor.

Are there APIs available to do that?

once Arbiter invokes your destructor

when can this happen? (outside of this code calling ArbiterFree())

Arbiter takes responsibility for user values as soon as they're passed into any Arbiter function. It will invoke the destructor when it's done with it.

It will invoke the destructor when it's done with it.

I didn't realize that. The way things are structured in this diff, the life of the user value is tied to the life of project identifier. That could lead to (possibly indefinite) delayed cleanup of the user value's resources, but I don't see how that would cause a crash. Do you?

If the Ruby object is GCed before Arbiter is done with the user value pointer, that could cause a crash. Since Arbiter treats user values (and project identifiers, versions, etc.) like, well, values, it'd be pretty easy for their lifetimes to not align.

The Correct Way™ to do this is remove the object from Ruby's GC while Arbiter is using it, then re-insert it afterward.

If the Ruby object is GCed before Arbiter is done with the user value pointer

The function project_identifier_mark() is there to prevent that from happening. I will rename it to *_gc_mark.

Question. You said:

Arbiter takes responsibility for user values as soon as they're passed into any Arbiter function. It will invoke the destructor when it's done with it.

But with the existence of ArbiterProjectIdentifierValue(), will the user value data be kept alive for as long as the ArbiterProjectIdentifier? This diff assumed this to be true.

will the user value data be kept alive for as long as the ArbiterProjectIdentifier?

Yes, but this isn't explicitly codified, and the value may outlive the project identifier if it gets copied behind-the-scenes.

jspahrsummers · 2016-09-18T22:20:22Z

bindings/ruby/extconf.rb

+
+require 'mkmf'
+
+$LDFLAGS << ' -lc++'


FWIW I ran into some problems on Linux trying to use this flag. I think you just need to compile with a C++ compiler if possible.

Yeah, this wouldn't work where there's only libstdc++. I thought about using clang++ but wasn't sure I want to enable the whole bundle of implicit flags turned on by that, vs the actual need of linking to a C++. I can use have_library() to check for libc++ first then try libstdc++ second.

the whole bundle of implicit flags turned on by that

What are those? Are they flags we would want anyways when linking a C++ library?

Good question, I'll check (with clang on OS X) tomorrow and report back.

Part of the concern was not what it is now, it was lack of visibility. Between compilers, between versions, between targets, the set of flags determined by the driver will change, but users aren't really aware of those flags, and how they change. Is it better to depend on the compiler always knowing best, or better to try to leave the compiler out and pass the one library to the linker?

Using Apple's clang that comes with Xcode 7.3.1, the difference between clang++ and clang is that clang++ adds the following compiler flags:

-fdeprecated-macro -fexceptions -fcxx-exceptions -stdlib=libc++

Enabling exceptions would make the binary bigger by some amount. I don't know if -stdlib=libc++ has any effect on compilation, or if it's more for header search paths and linking.

EDIT: Forgot -x c vs -x c++. I don't know where C/C++ differ in terms of spec, but I assume this Ruby extension wouldn't be impacted by differences in language semantics.

We actually want exceptions enabled when linking statically, because Arbiter relies upon them internally.

The "real" answer here is that Arbiter should be dynamically linked in most cases—I just set up static linking first because it was easier.

Why does enabling of exceptions depend on the linking?

I suppose that as long as Arbiter sources are compiled with -fexceptions, it shouldn't matter.

I just compiled with clang++ and there were 6 warnings and 11 errors. Also this message:

clang: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated

the warnings were "designated initializers are a C99 feature" and the errors were from the generic (i.e. varargs) function pointer type taken by rb_define_method() and others like it.

¯_(ツ)_/¯

This diff adds a `.destructor` callback to the user value received by ArbiterCreateProjectIdentifier(). This change is to provide precise lifetime control of those user values. This change isn't as simple as I would have liked it to be. The crux of these changes it that the user value is now additionally stored in the Ruby data object, so that the `.destructor` can nil it out. This one change resulted in most of the changes in this commit: 1. Introduction of `ProjectIdentifierData` 2. User value `.data` changed from `VALUE` to `VALUE *`

kastiglione added 4 commits September 17, 2016 20:15

Start Ruby extension

a5ea901

Start doing Arbiter::SemanticVersion

a46203d

Now Arbiter::Requirement

f833a23

Create dependencies

007a08e

kastiglione commented Sep 18, 2016

View reviewed changes

jspahrsummers reviewed Sep 18, 2016

View reviewed changes

kastiglione added 2 commits September 23, 2016 23:04

Fix argument type in project_identifier_mark

909e220

jspahrsummers mentioned this pull request Oct 6, 2016

[WIP] Haskell bindings #66

Open

Dave Lee added 3 commits October 7, 2016 22:27

Merge remote-tracking branch 'origin/master' into ruby-bindings

a93d7cd

Create basic Arbiter::SelectedVersion

486f7af

chmod u+x extconf.rb

1064ddb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Ruby bindings #67

[WIP] Ruby bindings #67

kastiglione commented Sep 18, 2016 •

edited

Loading

kastiglione commented Sep 18, 2016

kastiglione Sep 18, 2016

kastiglione Sep 24, 2016 •

edited

Loading

kastiglione Sep 18, 2016 •

edited

Loading

jspahrsummers Sep 18, 2016 •

edited

Loading

kastiglione Sep 20, 2016 •

edited

Loading

jspahrsummers Sep 24, 2016

kastiglione Sep 24, 2016

jspahrsummers Sep 24, 2016

jspahrsummers Sep 24, 2016

kastiglione Sep 25, 2016 •

edited

Loading

jspahrsummers Sep 26, 2016

jspahrsummers Sep 18, 2016

kastiglione Sep 18, 2016

jspahrsummers Sep 19, 2016

kastiglione Sep 19, 2016 •

edited

Loading

kastiglione Sep 20, 2016 •

edited

Loading

jspahrsummers Sep 24, 2016

kastiglione Sep 24, 2016

jspahrsummers Sep 24, 2016

kastiglione Sep 25, 2016 •

edited

Loading

jspahrsummers Sep 26, 2016 •

edited

Loading


		require 'mkmf'

		$LDFLAGS << ' -lc++'

[WIP] Ruby bindings #67

Are you sure you want to change the base?

[WIP] Ruby bindings #67

Conversation

kastiglione commented Sep 18, 2016 • edited Loading

kastiglione commented Sep 18, 2016

Choose a reason for hiding this comment

kastiglione Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

kastiglione Sep 18, 2016 • edited Loading

Choose a reason for hiding this comment

jspahrsummers Sep 18, 2016 • edited Loading

Choose a reason for hiding this comment

kastiglione Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kastiglione Sep 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kastiglione Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

kastiglione Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kastiglione Sep 25, 2016 • edited Loading

Choose a reason for hiding this comment

jspahrsummers Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

kastiglione commented Sep 18, 2016 •

edited

Loading

kastiglione Sep 24, 2016 •

edited

Loading

kastiglione Sep 18, 2016 •

edited

Loading

jspahrsummers Sep 18, 2016 •

edited

Loading

kastiglione Sep 20, 2016 •

edited

Loading

kastiglione Sep 25, 2016 •

edited

Loading

kastiglione Sep 19, 2016 •

edited

Loading

kastiglione Sep 20, 2016 •

edited

Loading

kastiglione Sep 25, 2016 •

edited

Loading

jspahrsummers Sep 26, 2016 •

edited

Loading