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

Extend load_from to allow loading from an absolute path #558

Closed
wants to merge 1 commit into from

Conversation

evnu
Copy link
Member

@evnu evnu commented Aug 17, 2023

load_from already has a format to specify a path relative to an OTP application. This commit extends the type for load_from to also allow setting an absolute path. Note that no special check for the validity of the path is being done, as we do not know beforehand what the target operating system looks like. Setting this configuration correctly is thus the user's responsibility.

Fix #556.

@evnu evnu requested a review from a team August 17, 2023 20:52
`load_from` already has a format to specify a path relative to an OTP
application. This commit extends the type for `load_from` to also allow
setting an absolute path. Note that no special check for the validity of
the path is being done, as we do not know beforehand what the target
operating system looks like. Setting this configuration correctly is
thus the user's responsibility.
@filmor
Copy link
Member

filmor commented Aug 23, 2023

Hmm, actually users should be able to just implement their own custom on_load handler right? I don't think it's a good idea to offer this in the library.

@evnu
Copy link
Member Author

evnu commented Aug 28, 2023

I don't think it's a good idea to offer this in the library.

You mean offering load_from at all, or the extension proposed here?

@filmor
Copy link
Member

filmor commented Aug 29, 2023

Passing the OTP application is fine, passing arbitrary paths is what I have an issue with. Nothing keeps users from just building their own on_load handler to achieve the same thing.

@evnu
Copy link
Member Author

evnu commented Aug 30, 2023

@filmor to check that I get you right, you propose that users overwrite @on_load and avoid using the rustler_init/0 function? That's prone to error as well I think. Erlang's :erlang.load_nif/2 function does not restrict the path where the NIF library resides, I am not sure if we should impose restriction on that path "artificially".

@filmor
Copy link
Member

filmor commented Aug 30, 2023

Yes... But that is exactly the problem with this PR as well. I suggest that if people want to load a NIF from an absolute path, they do it without use Rustler.

@evnu
Copy link
Member Author

evnu commented Aug 31, 2023

they do it without use Rustler.

That does not seem pratical. Users then also have to replicate the mechanism to auto-compile on changes with external resources and so on. I don't have a strong opinion on this topic, but I think users do not want to implement all of use Rustler just to load the library from a different path. But I see your point that using absolute paths introduces other problems for users. For example, the library is not necessarily versioned together with the application itself.

I will close this PR for now, we can discuss this further in #556 itself.

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.

Support for fully custom :load_from for escripts
2 participants