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

Add erlang-module support (update) #2806

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fnchooft
Copy link
Contributor

@fnchooft fnchooft commented Oct 1, 2024

Single modules are now allowed to be defined in an Erlang-cell.

In this case the entire code-block is interpreted as an erlang-module
and if there are no errors the module is compiled and loaded.

If the cell containing the module is deleted subsequent code invocations will fail.

Stale indicator is not working yet due to missing notion of functions, this will be fixed in a next version.

Copy link

github-actions bot commented Oct 1, 2024

Uffizzi Ephemeral Environment deployment-56782

☁️ https://app.uffizzi.com/github.com/livebook-dev/livebook/pull/2806

📄 View Application Logs etc.

What is Uffizzi? Learn more!

@fnchooft fnchooft force-pushed the erlang_module_support_v2 branch 3 times, most recently from c8b9e4f to f6865e4 Compare October 2, 2024 13:33
@fnchooft fnchooft marked this pull request as draft October 3, 2024 12:38
@fnchooft fnchooft force-pushed the erlang_module_support_v2 branch 3 times, most recently from de6b0fb to 27f7621 Compare October 3, 2024 17:53
Single modules are now allowed to be defined in an Erlang-cell.

In this case the entire code-block is interpreted as an
erlang-module and if there are no errors the module is compiled
and loaded.

If the cell containing the module is deleted subsequent code
invocations will fail.

Stale indicator is not working yet due to missing notion of
functions, this will be fixed in a next version.

Error handling - basics are working
 - Still not happy with it, but it is usable

The code is now pre-analyzed using the epp-module. This also
has the added advantage that erlang modules can become a bit
more expressive with defines and includes.

TODO: Examples need to be added to the example livebook.
@fnchooft
Copy link
Contributor Author

fnchooft commented Oct 3, 2024

Hi - I was able to invest a bit more time.

I went a different approach inspired by the epp-module.
The implementation is still erlang-heavy - but I am sure we can sprinkle some Elixir-sugar/goodies on-top.

As it is still work in progress I converted it into a draft.
I will add a livemd snipplet so people can see the potential. Later I would like to add those examples to the 'Welcome to Livebook'.

@fnchooft
Copy link
Contributor Author

fnchooft commented Oct 3, 2024

Livebook´s Erlang-module support feature

Test Elixir

This is a small example of how both Elixir- and Erlang-modules can be defined. The first implementation in Livebook for Erlang only allowed Erlang-statements.

Hopefully this extension will add some tools for those of us in Erlang-land, while we tinker and learn 'from' and 'about' our younger cousin Elixir.

defmodule Bench do
  def sum do
    Enum.reduce(1..1_000_000, 0, fn x, acc -> x + acc end)
  end
end
Bench.sum()

First Erlang-module

-module(go).
-export([go/0, went/0]).
-export([gone/0]).

go() -> {ok,go}.
went() -> {ok,went}.
gone() -> {ok,gone}.

Calling the functions can now be done using an Erlang-cell , using Erlang-syntax:

{ok,Go} = go:go(),
{ok,Went} = go:went(),
{ok,Gone} = go:gone(),
{Go,Went,Gone}.

Error-handling

In order to see how you get feedback about errors in your code, start uncommenting the lines below:

-module(go2).
-export([go/0, went/0]).
-export([gone/0]).

go() -> {ok,go}.
went() -> {ok,went}.
gone() -> {ok,gone}.

%module(go3).
%-export([go/0]).
%go() -> {ok,go}.

Macro-expansion and other goodies

The first version of the implementation only dealt with very simple erlang-modules, however we should be able to use all the keywords from Erlang right? Below the first attempt where macro-expansion work when defined locally:

-module(my_macro_example).
-define(HELLO_MSG, "Hello, Erlang!").

-export([greet/0]).

-spec greet() -> ok.
greet() ->
    io:format("~s~n", [?HELLO_MSG]).
A = 1,
B = 2.
{A,my_macro_example:greet(),B}.

More interresting use-cases

Now after we validated that 'simple stuff' works - lets get a snipplet of code from the OTP-testsuites: https://github.com/erlang/otp/blob/master/lib/xmerl/test/xmerl_test_lib.erl.

-module(xmerl_test_lib).

-compile(export_all).

-include_lib("common_test/include/ct.hrl").
-include_lib("xmerl/include/xmerl.hrl").

%% cmp_element/2
%% First argument result after parsing
%% Second argument result after validation
cmp_element(E,E) ->
    ok;
cmp_element(#xmlElement{name=N,attributes=A1,content=C1},
	    #xmlElement{name=N,attributes=A2,content=C2}) ->
    case cmp_attributes(A1,A2) of
	ok ->
	    cmp_elements(C1,C2);
	Err -> Err
    end;
cmp_element(#xmlText{},#xmlText{}) ->
    ok;
cmp_element(A,B) ->
    {error,{A,does_not_match,B}}.

cmp_elements([H1|T1],[H2|T2]) ->
    case cmp_element(H1,H2) of
	ok ->
	    cmp_elements(T1,T2);
	Err ->
	    Err
    end;
cmp_elements([],[]) ->
    ok.

%% All attributes in argument 1 must be present in 2
cmp_attributes([A1|T1],Atts2) ->
    case keysearch_delete(A1#xmlAttribute.name,#xmlAttribute.name,Atts2) of
	{A2,NewAtts2} ->
	    case A1#xmlAttribute.value == A2#xmlAttribute.value of
		true ->
		    cmp_attributes(T1,NewAtts2);
		_ ->
		    {error,{mismatching_values_in_attsibutes,A1,A2}}
	    end;
	_ ->
	    {error,{no_matching_attsibute,A1,in,Atts2}}
    end;
cmp_attributes([],_) ->
   ok.

keysearch_delete(Key,N,List) ->
    case lists:keysearch(Key,N,List) of
	{value,Res} ->
	    {Res,lists:keydelete(Key,N,List)};
	_ ->
	    false
    end.

Todos

No implementation is ever 'done' - following items should and could be implemented in a next phase:

  • Stale indicator

    • If you alter a module - you would want to know about it right?
      • Elixir cell show a yellow-stale icon - Erlang code should do the same.
  • Code completion

    • Erlang is starting to get super edoc-support - we should be able to leverage it
  • Include files

    • Have not found a super-way yet - but sometimes you would like to add some definitions

@fnchooft fnchooft mentioned this pull request Oct 3, 2024
9 tasks
@jonatanklosko
Copy link
Member

jonatanklosko commented Oct 4, 2024

I went a different approach inspired by the epp-module.

@fnchooft does it improve anything in terms of supported features (compared to the previous approach)? My main concern is that now we write to a temporary file, which wouldn't be possible on a readonly file system, as is the case with nerves_livebook (running Livebook embedded devices).

@josevalim
Copy link
Contributor

@jonatanklosko I believe it gives us macros, includes, and a bunch of other stuff. We should probably add support for strings into epp, if it only requires files for now.

@josevalim
Copy link
Contributor

@jonatanklosko oh, I have an idea. epp accepts a fd option, so we can probably pass Elixir's StringIO with the file contents as argument, without the file in disk. We can give it a try after merging.

@jonatanklosko
Copy link
Member

@josevalim smart, let's try that!


defp eval_erlang_module(_code, module, forms, binding, env) do
try do
case :compile.forms(forms) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can pass the options return_errors and return_warnings, so it returns errors and warnings and convert them to diagnostics. But I think we can do it in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a case - later we should clean it up as you indicated.

@fnchooft
Copy link
Contributor Author

fnchooft commented Oct 4, 2024

I went a different approach inspired by the epp-module.

@fnchooft does it improve anything in terms of supported features (compared to the previous approach)? My main concern is that now we write to a temporary file, which wouldn't be possible on a readonly file system, as is the case with nerves_livebook (running Livebook embedded devices).

As @josevalim indicated: Macros, Defines the whole lot. The proposed solution based on fd seems a great direction.

Agree - no need to be very verbose.

Co-authored-by: José Valim <[email protected]>
@fnchooft
Copy link
Contributor Author

fnchooft commented Oct 4, 2024

If you guys agree - Ready for review this draft?

Comment on lines 629 to 632
# TODO: Make sure to change this into a folder which pertains the session.
defp tmp_filename() do
:lists.flatten(:io_lib.format("/tmp/erlang-eval-~p.tmp", [:erlang.phash2(make_ref())]))
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We get tmp_dir on init, so we could use that as the base. But let's try the fileless approach first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fileless approach won't work.

@fnchooft we pass a tmp_dir to init:

tmp_dir = Keyword.get(opts, :tmp_dir)

Can you store it as part of the state and use it to build the tmp directory? Notice that it may be nil in case of read-only file systems, so we should return an error in those cases saying you can't define erlang modules in read-only file systems. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do - some subfolder was not created ( added a File.mkdir_p )

try do
case :compile.forms(forms) do
{:ok, _, binary_module} ->
{:module, module} = :code.load_binary(module, ~c"#{module}.beam", binary_module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically :code.load_binary doesn't use the filename, but if we want to be accurate, we can do this:

Suggested change
{:module, module} = :code.load_binary(module, ~c"#{module}.beam", binary_module)
file =
if ebin_path = ebin_path() do
Path.join(ebin_path, "#{module}.beam")
else
"#{module}.beam"
end
{:module, module} = :code.load_binary(module, String.to_charlist(file), binary_module)

@fnchooft
Copy link
Contributor Author

fnchooft commented Oct 4, 2024

@jonatanklosko oh, I have an idea. epp accepts a fd option, so we can probably pass Elixir's StringIO with the file contents as argument, without the file in disk. We can give it a try after merging.

@josevalim - I know you tried it - however we are coming back to erlang/otp#7239 - in order to get proper OTP ram-file support.

A ram-file (https://github.com/erlang/otp/blob/master/lib/kernel/src/ram_file.erl) exists but not all modules know how to deal with it.

History is telling:

Erlang-statement do not need a tmp_dir.

Erlang-modules do, so find out if it is a module without epp.

Cleanup of tests.

Cleanup of unused variables.
@@ -23,7 +23,8 @@ defmodule Livebook.Runtime.EvaluatorTest do
send_to: self(),
object_tracker: object_tracker,
client_tracker: client_tracker,
ebin_path: ebin_path
ebin_path: ebin_path,
tmp_dir: "/tmp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make this tmpdir specific to the test. Also, it should be enabled explicitly with a tag, we can use the existing @tag :with_ebin_path. This way we can test the error when no tmpdir is available.

So you can change the setup block to this:

setup ctx do
  {tmp_path, ebin_path} =
    if ctx[:with_ebin_path] do
      hash = ctx.test |> to_string() |> :erlang.md5() |> Base.encode32(padding: false)
      tmp_path = ["tmp", inspect(ctx.module), hash] |> Path.join() |> Path.expand()
      ebin_path = Path.join(tmp_path, "ebin")
      File.rm_rf!(ebin_path)
      File.mkdir_p!(ebin_path)
      Code.append_path(ebin_path)
      {tmp_path, ebin_path}
    end

  {:ok, object_tracker} = start_supervised(Evaluator.ObjectTracker)
  {:ok, client_tracker} = start_supervised(Evaluator.ClientTracker)

  opts = [
    send_to: self(),
    object_tracker: object_tracker,
    client_tracker: client_tracker,
    tmp_path: tmp_path,
    ebin_path: ebin_path
  ]

  {:ok, _pid, evaluator} = start_supervised({Evaluator, opts})

  %{
    evaluator: evaluator,
    object_tracker: object_tracker,
    client_tracker: client_tracker,
    tmp_path: tmp_path,
    ebin_path: ebin_path
  }
end

Then add @tag :with_ebin_path above each test that needs to have the tmpdir :)

Comment on lines +439 to +440
tmp_dir = Map.get(state, :tmp_dir)
{eval_result, code_markers} = eval(language, code, context.binding, context.env, tmp_dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tmp_dir = Map.get(state, :tmp_dir)
{eval_result, code_markers} = eval(language, code, context.binding, context.env, tmp_dir)
{eval_result, code_markers} =
eval(language, code, context.binding, context.env, state.tmp_dir)

Comment on lines +746 to +748
# It is an ugly hack for now - need to solve https://github.com/erlang/otp/issues/7239
filename = String.to_charlist(tmp_filename(tmp_dir))
:file.write_file(filename, code)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# It is an ugly hack for now - need to solve https://github.com/erlang/otp/issues/7239
filename = String.to_charlist(tmp_filename(tmp_dir))
:file.write_file(filename, code)
# Consider using in-memory file, once :ram file supports IO device API.
# See https://github.com/erlang/otp/issues/7239
filename = Path.join(tmp_dir, "epp#{random_long_id()}")
File.write!(filename, code)

Comment on lines +781 to +782
# Clean up after ourselves.
:file.delete(filename)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Clean up after ourselves.
:file.delete(filename)
_ = File.rm(filename)

Comment on lines +631 to +647
# TODO: Make sure to change this into a folder which pertains the session.
#
# Better alternative would be to get proper OTP ram-file support.
# https://github.com/erlang/otp/issues/7239 - Request for ram-option.
#
# https://github.com/erlang/otp/blob/master/lib/kernel/src/ram_file.erl
# a ram-file exists but not all modules know how to deal with it.
#
# History is telling:
# https://erlang.org/pipermail/erlang-questions/2007-March/025623.html
# https://erlang.org/pipermail/erlang-questions/2002-August/005562.html
# https://github.com/ebengt/erlang_string_io
#
defp tmp_filename(tmp_dir) do
tmp_id = :erlang.phash2(make_ref())
Path.join(tmp_dir, "erlang-eval-#{tmp_id}.tmp")
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# TODO: Make sure to change this into a folder which pertains the session.
#
# Better alternative would be to get proper OTP ram-file support.
# https://github.com/erlang/otp/issues/7239 - Request for ram-option.
#
# https://github.com/erlang/otp/blob/master/lib/kernel/src/ram_file.erl
# a ram-file exists but not all modules know how to deal with it.
#
# History is telling:
# https://erlang.org/pipermail/erlang-questions/2007-March/025623.html
# https://erlang.org/pipermail/erlang-questions/2002-August/005562.html
# https://github.com/ebengt/erlang_string_io
#
defp tmp_filename(tmp_dir) do
tmp_id = :erlang.phash2(make_ref())
Path.join(tmp_dir, "erlang-eval-#{tmp_id}.tmp")
end

Comment on lines +766 to +770
:error ->
{{:error, :error, "compile forms error", []}, []}

{:error, _errors, _warnings} ->
{{:error, :error, "compile forms error (warnings)", []}, []}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It returns only either of these, depending on whether we pass :return_errors or not. If the plan is to do it in a separate PR, let's leave a TODO :)

Suggested change
:error ->
{{:error, :error, "compile forms error", []}, []}
{:error, _errors, _warnings} ->
{{:error, :error, "compile forms error (warnings)", []}, []}
# TODO: compile with :return_errors, :return_warnings and convert
# them to diagnostics
:error ->
{{:error, :error, "compile forms error", []}, []}

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.

3 participants