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 no-caml-startup feature flag that avoids the caml_startup symbol #37

Merged
merged 1 commit into from
Aug 16, 2021

Conversation

tizoc
Copy link
Owner

@tizoc tizoc commented Aug 14, 2021

This will make OCamlRuntime::init_persistent() a noop.

The main motivation for this feature flag is to make code that uses
ocaml-rs and ocaml-interop loadable from dune utop:

zshipko/ocaml-rs#70

})
#[cfg(not(feature = "no-caml-startup"))]
{
static INIT: std::sync::Once = std::sync::Once::new();

Choose a reason for hiding this comment

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

Shouldn't this function always call caml_startup? As written, calling init above, allowing its result to be dropped (calling caml_shutdown) and then calling init again will return an OCamlRuntime not backed by an available runtime.

caml_startup will already avoid re-initialising if it is already running (and bumps a counter, to allow a corresponding caml_shutdown to behave non-destructively), so I believe it should be safe and more correct to simply delete this.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It should never be called from an OCaml program that wants to use Rust code, hence the panic. You only do this initialization once in your program, from your main function or somewhere close to it (you may for example have a separate thread that has exclusive access to the OCaml runtime, in that case you would do it there). But no function exposed to OCaml should be doing this.

Copy link
Owner Author

@tizoc tizoc Aug 16, 2021

Choose a reason for hiding this comment

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

Remember that this is nothing else more a workaround for the dune utop issue, the panic is in there just as a safeguard (to help anyone misusing the library), but this shouldn't be called in such case.

Copy link

@mrmr1993 mrmr1993 Aug 16, 2021

Choose a reason for hiding this comment

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

I don't think this is enforced anywhere: you could write

fn call_first() {
  let r1 = init();
  ...
}
fn call_snd() {
  let r2 = init();
  ...
}
pub fn main() {
  call_first();
  call_snd();
}

and the second call won't get an OCaml runtime. I suppose this can be a separate issue.

Edit: opened #39 for this.

@tizoc tizoc force-pushed the no-caml-startup-feature branch from 79a3f8c to e70b2b0 Compare August 16, 2021 02:45
Copy link

@mrmr1993 mrmr1993 left a comment

Choose a reason for hiding this comment

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

Does it make sense to update the doc-comment for recover_handle too? In particular, it's the only way to recover a Runtime from calls out of an existing runtime.

(It would also be nice if ocaml-rs could then allow a &Runtime argument to functions that calls this automatically, so that it's user-friendly to release the runtime lock, but that's obviously not a change for this PR.)

@tizoc
Copy link
Owner Author

tizoc commented Aug 16, 2021

@mrmr1993 I have to double-check, but the way I remember it, ocalm-rs has a default name for the runtime handle, and a way to specify if you with the attribute annotation used for functions expose to OCaml. That is how you obtain the handle when called from OCaml (in ocaml-interop it is just the first argument to the function and you have to name it explicitly).

@tizoc
Copy link
Owner Author

tizoc commented Aug 16, 2021

@mrmr1993 check here https://github.com/zshipko/ocaml-rs/blob/ff07e873ed502eb25b8429a99c86cc831e4a7d1f/derive/src/lib.rs#L173

By default the name is gc, but you can override it from the example I see on the docs:

/// A name for the garbage collector handle can also be specified:
#[cfg(feature = "derive")]
#[ocaml::func(my_gc_handle)]
pub unsafe fn my_string() -> ocaml::Value {
    ocaml::Value::string("My string")
}

@tizoc tizoc force-pushed the no-caml-startup-feature branch from e70b2b0 to 4c27368 Compare August 16, 2021 03:00
@tizoc
Copy link
Owner Author

tizoc commented Aug 16, 2021

@zshipko btw, although it is likely to be a temporary workaround, you may want to document this somewhere in ocaml-rs documentation (for ocaml-interop it doesn't really do anything because unlike ocaml-rs, ocaml-interop doesn't support bytecode functions yet).

…bol.

Will also be enabled if the `OCAML_INTEROP_NO_CAML_STARTUP` environment variable is set.

This will make `OCamlRuntime::init_persistent()` a noop.

The main motivation for this feature flag is to make code that uses
ocaml-rs and ocaml-interop loadable from `dune utop`:

zshipko/ocaml-rs#70
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.

2 participants