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

macOS App includes elixir, erlang for standalone mode. #929

Merged
merged 11 commits into from
Jan 24, 2022

Conversation

paridin
Copy link
Contributor

@paridin paridin commented Jan 23, 2022

This PR solves issue #908

  • Embed the release with include_erts: false
  • Copy Erlang into the release
  • Copy Elixir into the release
  • Add both to the path before starting Livebook

Screen Shot 2022-01-23 at 14 27 41

@josevalim
Copy link
Contributor

This looks amazing, thank you! Just one major note: the copying code is not really part of the AppBuilder. Our goal is to make AppBuilder a separate project and I think the need to copy Elixir and Erlang as separate executables is Livebook specific as it executes code. My suggestion is to create at "rel/app/standalone.exs" that defines both Standalone.erts and Standalone.elixir. In the mix.exs, inside standalone_erlang_elixir, you can do this:

  Code.require_file("rel/app/standalone.exs")
  Standalone.copy_elixir(...)
  Standalone.copy_erlang(...)

WDYT?

mix.exs Outdated Show resolved Hide resolved
@paridin
Copy link
Contributor Author

paridin commented Jan 23, 2022

This looks amazing, thank you! Just one major note: the copying code is not really part of the AppBuilder. Our goal is to make AppBuilder a separate project and I think the need to copy Elixir and Erlang as separate executables is Livebook specific as it executes code. My suggestion is to create at "rel/app/standalone.exs" that defines both Standalone.erts and Standalone.elixir. In the mix.exs, inside standalone_erlang_elixir, you can do this:

  Code.require_file("rel/app/standalone.exs")
  Standalone.copy_elixir(...)
  Standalone.copy_erlang(...)

WDYT?

Looks good to me, but to keep clear my mind

The elixir, erlang source will be inside of the released app on paths rel/elixir, rel/erts-XX.X?

and the code to copy in those paths will be moved to rel/app/standalone.exs file and called via.

  Code.require_file("rel/app/standalone.exs")
  Standalone.copy_elixir(...)
  Standalone.copy_erlang(...)

@josevalim
Copy link
Contributor

The elixir, erlang source will be inside of the released app on paths rel/elixir, rel/erts-XX.X?

You can copy those to any directory you want inside the release. To avoid confusion with release artifacts, I would personally copy them to the vendor directory inside the release. We won't have the source in the repository, they will still be copied exactly how you did. :D

The only thing you should move around are the copy functions to the PROJECT_ROOT/rel/app/standalone.exs.

Does that make sense? So, just to be clear, you should just move the code around. The functionality is all here already. :)

@paridin
Copy link
Contributor Author

paridin commented Jan 23, 2022

I understood, thank you. I will back with the changes.

@paridin
Copy link
Contributor Author

paridin commented Jan 23, 2022

image

rel/app/standalone.exs Outdated Show resolved Hide resolved
@josevalim
Copy link
Contributor

@paridin lovely! Just two additional comments and we are ready to ship it!

@paridin
Copy link
Contributor Author

paridin commented Jan 24, 2022

@josevalim One doubt related to elixir from /releases/<version>/elixir. In the previous commits, I checked that it would work. That's why I was patching this file.

With the new changes, we must move the libs for erlang to be part of the vendor to keep the environment structure and all works as expected.

After learning for try and error I can say that this error is because the libs are missing. init terminating in do_boot ({undef,[{elixir,start_cli,[],[]},{init,start_em,1,[]},{init,do_boot,3,[]}]}), that error comes from call /releases/<version>/elixir.

Without discovering how to append the library path to search on the correct directory, I can't get it working. But that effort looks irrelevant since I checked with System.find_executable/1 on both modes, the standalone and embedded, and they point to /rel/vendor/{erts,elixir}/bin respectively, which is what we want.

Since /releases/<version>/elixir is set via :assemble step, I would like it is possible to know the purpose and more relevant if the error before mentioned will do not cause any problem.

image
image

mix.exs Outdated Show resolved Hide resolved
rel/app/standalone.exs Outdated Show resolved Hide resolved
rel/app/standalone.exs Outdated Show resolved Hide resolved
@josevalim
Copy link
Contributor

Beautiful, thank you!

@paridin
Copy link
Contributor Author

paridin commented Jan 24, 2022

I think it is done this is the output structure
image

While moving the erlang, I see that the erts binaries always look for ROOT/bin and ROOT/lib, and the boot files must be in ROOT/bin.

Copy link
Contributor

@wojtekmach wojtekmach left a comment

Choose a reason for hiding this comment

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

This is fantastic, just two comments below!

Comment on lines +137 to +139
additional_paths = additional_paths
|> Enum.map(&("\\(resourcePath)#{&1}"))
|> Enum.join(":")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
additional_paths = additional_paths
|> Enum.map(&("\\(resourcePath)#{&1}"))
|> Enum.join(":")
additional_paths = Enum.map_join(additional_paths, ":", &"\\(resourcePath)#{&1}")

def copy_elixir(release, elixir_version) do
# download and unzip
standalone_destination = Path.join(release.path, "vendor/elixir")
download_elixir_at_destination(standalone_destination, elixir_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we're already downloading Elixir in out bootstrap script https://github.com/livebook-dev/livebook/blob/main/.github/scripts/app/bootstrap_mac.sh#L123:L124 so we don't need to do it again. I think it's better if this function really just copies the currently running Elixir, similarly how copy_erlang does that for OTP. This way it's going to be super easy to test it with Elixir main when developing too.

Copy link
Contributor

@josevalim josevalim Jan 24, 2022

Choose a reason for hiding this comment

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

I am worried though that we will have even less control over how Elixir is installed. For example, some package installers like to change the bin files. :'(

The downside of using the Elixir precompiled though is that it uses the minimum OTP version (at least until Elixir v1.14) and if you are building from scratch then it is guaranteed you compiled it to the latest.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair. But we'll always be building Livebook.app with our bootstrap script where we have full control so I don't think that's gonna be an issue in practice. We need to be running the bootstrap script because we need to have full control over OTP for exactly same reason.

@@ -131,11 +133,20 @@ defmodule Livebook.MixProject do
version: @version,
logo_path: "static/images/logo.png",
url_schemes: ["livebook"],
additional_paths: ["/rel/vendor/bin", "/rel/vendor/elixir/bin"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bit confusing that we have vendor/elixir/bin for Elixir and vendor/bin for Erlang. In that sense I think what we previously had, vendor/erts/bin, made more sense to me.

I think this:

Or perhaps we should nest all of Erlang under rel/vendor/erlang...

(#929 (comment))

would make even more sense actually. We're vendoring in all of OTP, not just binaries but also libraries. If we do that then rel/lib would only contain Livebook and its Mix dependencies but no OTP or Elixir libraries, those would be in vendor. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

vendor/erts/bin, made more sense to me.

This may not work as the scripts in erts/bin may not be the same as in bin.

If we do that then rel/lib would only contain Livebook and its Mix dependencies but no OTP or Elixir libraries

Unfortunately this may not work either as Elixir releases modify the Elixir executables and it can change in further ways in the future.

The current approach does duplicate Elixir but it is fine because it is small. Plus the non-vendored version has stripped beams.

@wojtekmach wojtekmach merged commit 5110f85 into livebook-dev:main Jan 24, 2022
@wojtekmach
Copy link
Contributor

Yup, please ignore my comments. Thank you!

@paridin paridin deleted the macos-app-version branch January 24, 2022 22:44
@paridin
Copy link
Contributor Author

paridin commented Jan 24, 2022

Thank you, guys; I learned a lot from this.

@w0rd-driven
Copy link
Contributor

I also wanted to thank everyone as I hadn't fully understood releases, so I couldn't quite fit all of the pieces together until I saw it laid out like this. I learned a lot by watching :>.

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.

4 participants