Repository navigation
feat: add load_data_fun to load dynamic data - #107
Conversation
philss
left a comment
philss
left a comment
There was a problem hiding this comment.
Hi @leandrocp 👋
Thanks for the PR! Please check the comment and the CI.
| assert {:module, ^module} = :code.load_binary(module, ~c"nofile", binary) | ||
| assert_receive :load_data_called | ||
| end) | ||
| end |
There was a problem hiding this comment.
@leandrocp This test is giving a SIGSEGV for me (like it does on CI), and I couldn't figure out how to fix it.
Please give it a try, but if you can't, I'm comfortable with proceeding without this integration test.
There was a problem hiding this comment.
Hey @philss I was able to reproduce on a Linux container (it works on my local Mac). I think the root cause was caused by 051306b where it added the archive extension so File.rm/1 stopped working silently and the tests in this PR tries to reload an existing NIF and it crashes. But I can't explain why it works in some envs but not others so I'm not completely sure.
Note that 6e7d7bc matches File.rm/1 returns. I think it's correct but I'm happy to make more changes! Thanks for your time reviewing this!
There was a problem hiding this comment.
I see, and I agree it looks correct. Would you mind to open a isolated PR with that commit? I think it would be better for documenting the fix. We can rebase it after the merge.
There was a problem hiding this comment.
It's odd that we don't have a conflict here, but please rebase so I can merge with only the changes for the load_data_fun.
There was a problem hiding this comment.
I applied the same patch in #109 and just added one test so I think that wouldn't create a conflict :)
|
@leandrocp thank you for your efforts here! Great work! I should release a new version (minor) between today and tomorrow. |
|
@leandrocp there is a new release, v0.10.0, with your changes :) |
|
Thanks @philss ! |
2 participants
Closes #54
Hey @philss I found a situation where I needed to load data dynamically and since #54 is open I'm pushing this proposal.
Disclaimer: used claude but reviewed all changes and tested the config on my own projects.