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.

Sign up for GitHub

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

Jump to bottom

Extend load_from to allow loading from an absolute path #558

Closed
wants to merge 1 commit into from
Closed

Extend load_from to allow loading from an absolute path #558

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
@evnu
Extend load_from to allow loading from an absolute path
bb693f2 
`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.

@evnu evnu closed this Aug 31, 2023
@evnu evnu mentioned this pull request Aug 31, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for fully custom :load_from for escripts
2 participants
@evnu @filmor