-
Notifications
You must be signed in to change notification settings - Fork 5
Add escriptize provider #59
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
base: master
Are you sure you want to change the base?
Conversation
Requires support for AtomVM: atomvm/AtomVM#1948 NB: it's currently incompatible with JIT Signed-off-by: Paul Guyot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still reviewing some of the more complex sections, but I have a few simple fixes and requests that I noticed in a first pass. It would also be good to have some tests to verify the assembled escript.
| -define(PROVIDER, escriptize). | ||
| -define(DEPS, [packbeam]). | ||
| -define(OPTS, [ | ||
| {atomvm_binary, $b, "atomvm_binary", string, "Path to AtomVM binary (default: which AtomVM)"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be which atomvm, see comment below.
| PossiblePaths = [ | ||
| "/opt/local/lib/atomvm/atomvmlib.avm", | ||
| "/usr/local/lib/atomvm/atomvmlib.avm", | ||
| "/usr/lib/atomvm/atomvmlib.avm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest $HOME/.local/lib/atomvm/atomvm.lib added to this list for unprivileged users who install into their local user environment.
| %% @private | ||
| find_atomvmlib() -> | ||
| % First try to infer from AtomVM wrapper script | ||
| case os:find_executable("AtomVM") of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the standard install using make install or ninja install does not place the AtomVM binary in PATH, just the atomvm launcher scrtipt.
| -define(DEPS, [packbeam]). | ||
| -define(OPTS, [ | ||
| {atomvm_binary, $b, "atomvm_binary", string, "Path to AtomVM binary (default: which AtomVM)"}, | ||
| {atomvmlib, $l, "atomvmlib", string, "Path to atomvmlib.avm"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets also add (default: auto-detected if AtomVM is installed)
| rebar_api:error( | ||
| "An error occurred in the ~p task. Class=~p Error=~p Stacktrace=~p~n", [ | ||
| ?PROVIDER, C, E, S | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other providers I am trying to update catch C:E:S to not display Stacktrace=~p~ in the rebar_api:error/2 message but add a rebar_api:debug/2 log after that will display the stacktrace in diagnostic mode. It makes the trace much easier to read when it starts on a new line, otherwise it is pushed so far to the right of the screen most of the output lines wrap in a hard to follow manner. I also don't think these stackreaces bring a lot of value to end users by default, they are more helpful for contributors familiar with the plugin. Most rebar3 plugins do not display stacktraces unless using diagnostic mode.
Requires support for AtomVM:
atomvm/AtomVM#1948
NB: it's currently incompatible with JIT