Add dynamic load data

[kaaboaye]

Makes it possible to compute load_data at the runtime.

Example use case it to provide priv path to the nif.

defmodule NIF do
  use Rustler, load_data_fun: {Deployment, :nif_data}
end

defmodule Deployment do
  def nif_data do
    :code.priv_dir(:surfer_local) |> IO.iodata_to_binary()
  end
end

Also added explicit errors to indicate that user provided invalid config.

[hansihe]

I like the idea here, but I think dynamic load_data should be a separate argument, something like load_data_fun?

As is, this will break existing load_datas that are atom pairs.

[kaaboaye]

Sure, I'll update it in the free time and ping you

[kaaboaye]

@hansihe refactored and tested on my project

[evnu]

@kaaboaye do you think you could add a test for this? In rustler_test, any NIF could also load some data with that function. We could then test that this data is available.

EDIT: Also, I fixed clippy on master, so rebasing should fix clippy problems here as well I think.

[kaaboaye]

I've added test making use of dynamic load.

However, is this output with a couple of panics is expected?

[evnu]

Thanks for the tests! Yes, the output is expected.

[evnu]

The pipeline complains on windows:

 error[E0433]: failed to resolve: could not find `unix` in `os`
 --> rustler_tests\native\dynamic_load\src/lib.rs:2:47
Warning:   |
2 | use std::{ffi::OsStr, fs::read_to_string, os::unix::prelude::OsStrExt, path::PathBuf};
  |                                               ^^^^ could not find `unix` in `os`

error[E0599]: no function or associated item named `from_bytes` found for struct `OsStr` in the current scope
  --> rustler_tests\native\dynamic_load\src/lib.rs:32:28
Warning:    |
32 |     let priv_path = OsStr::from_bytes(priv_path);
   |                            ^^^^^^^^^^ function or associated item not found in `OsStr`

[kaaboaye]

I can fix this test for Windows but I could figure out some other case which doesn’t rely on platform specific API.

Windows fix would work as an example usage of this feature with real world use case.

but it would also mean that when adding support to other platforms this test could fail again

Should I fix it for windows or figure out something different?

[evnu]

Should I fix it for windows or figure out something different?

I think fixing it for windows is sufficient. We have runners for linux, macos and windows, it suffices to only make them work for now.

[kaaboaye]

Looking into the issue of initialization I've realized that those modules can be also unloaded while updating. erl_nif exposes unload function for that purpose. It's mainly development issue. Hot module reloads will leak the DATASET unless upgrade and/or unload is provided.

So maybe instead of making sure that the DATASET is initialized only once we should make sure that upgrades are properly handled?

Does anyone here have any experience with how upgrades work? Does the library get new "static" memory space or is it reusing the old memory?

[evnu]

Does anyone here have any experience with how upgrades work? Does the library get new "static" memory space or is it reusing the old memory?

We have a long-standing issue w.r.t. upgrades: #13. Currently, upgrading is not allowed.

So maybe instead of making sure that the DATASET is initialized only once we should make sure that upgrades are properly handled?

As upgrades are not supported right now, we can probably revisit that part later on. But maybe we need to figure out how to handle unload. I think this is probably not called usually, but only if an upgrade will happen?

[kaaboaye]

But maybe we need to figure out how to handle unload.

I think unsafe { DATASET = None } should be enough. Rust should drop Some(dataset) after that. Assuming that OTP will wait with unload until all pending function calls return.

[evnu]

Ok, so only the test on windows is left here I think.

[kaaboaye]

Added windows support, all tests are green on windows now.

[evnu]

@kaaboaye thanks for all your work! Can you try to rebase one more time on master? We had some changes to the CI, which should make the tests run more reliably. With that, I think we can then merge here.

[kaaboaye]

Done

[evnu]

Seems that there is a merge conflict, can you resolve that?

[evnu]

@philss resolved the merge conflict, thanks!