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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- `no-caml-startup` feature flag that disables calls to `caml_startup` when initializing the runtime. This makes `OCamlRuntime::init_persistent()` a noop. It is useful for being able to load code that uses `ocaml-rs` in an utop toplevel (for example, when running `dune utop`). Will be enabled if the environment variable `OCAML_INTEROP_NO_CAML_STARTUP` is set. This is a temporary option that is likely to be removed in the future once a better solution is implemented.

## [0.8.4] - 2021-05-18

### Added
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@ static_assertions = "1.1.0"
[features]
without-ocamlopt = ["ocaml-sys/without-ocamlopt", "ocaml-boxroot-sys/without-ocamlopt"]
caml-state = ["ocaml-sys/caml-state"]
no-caml-startup = []
14 changes: 14 additions & 0 deletions build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) Viable Systems and TezEdge Contributors
// SPDX-License-Identifier: MIT

const OCAML_INTEROP_NO_CAML_STARTUP: &str = "OCAML_INTEROP_NO_CAML_STARTUP";

fn main() {
println!(
"cargo:rerun-if-env-changed={}",
OCAML_INTEROP_NO_CAML_STARTUP
);
if std::env::var(OCAML_INTEROP_NO_CAML_STARTUP).is_ok() {
println!("cargo:rustc-cfg=feature=\"no-caml-startup\"");
}
}
28 changes: 16 additions & 12 deletions src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
// SPDX-License-Identifier: MIT

use ocaml_boxroot_sys::{boxroot_setup, boxroot_teardown};
use ocaml_sys::{caml_shutdown, caml_startup};
use std::{marker::PhantomData, sync::Once};
use std::marker::PhantomData;

use crate::{memory::OCamlRef, value::OCaml};

Expand All @@ -30,15 +29,20 @@ impl OCamlRuntime {
///
/// After the first invocation, this method does nothing.
pub fn init_persistent() {
static INIT: Once = Once::new();

INIT.call_once(|| {
let arg0 = "ocaml\0".as_ptr() as *const ocaml_sys::Char;
let c_args = vec![arg0, core::ptr::null()];
unsafe {
caml_startup(c_args.as_ptr());
}
})
#[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.


INIT.call_once(|| {
let arg0 = "ocaml\0".as_ptr() as *const ocaml_sys::Char;
let c_args = vec![arg0, core::ptr::null()];
unsafe {
ocaml_sys::caml_startup(c_args.as_ptr());
}
})
}
#[cfg(feature = "no-caml-startup")]
panic!("Rust code that is called from an OCaml program should not try to initialize the runtime.");
}

/// Recover the runtime handle.
Expand Down Expand Up @@ -77,7 +81,7 @@ impl Drop for OCamlRuntime {
fn drop(&mut self) {
unsafe {
boxroot_teardown();
caml_shutdown();
ocaml_sys::caml_shutdown();
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion testing/ocaml-caller/rust/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ crate-type = ["staticlib", "cdylib"]

[dependencies]
ocaml-interop = { path = "../../../../.." }
# Above is for building with dune, bellow is for biulding from this directory
# Above is for building with dune, bellow is for building from this directory
# ocaml-interop = { path = "../../.." }