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

Run Livebook Desktop without EPMD #2591

Merged
merged 15 commits into from
May 8, 2024
Merged

Run Livebook Desktop without EPMD #2591

merged 15 commits into from
May 8, 2024

Conversation

josevalim
Copy link
Contributor

@josevalim josevalim commented May 6, 2024

If EPMD is running, we will register ourselves within EPMD, otherwise we don't bother starting it. This works by passing the parent port to the runtime which then matches against it when trying to connect back. We also patch the epmd module to keep the distribution port (which otherwise we wouldn't know).

To test this, you can killall epmd (or epmd -kill), start Livebook, and double check running a notebook works as expected. There are some pending features which I will need the power of friendship to solve:

  • Test the current PR works on Windows
  • Test the current PR works on macOS

Copy link

github-actions bot commented May 6, 2024

Uffizzi Preview deployment-51322 was deleted.

@@ -393,6 +418,8 @@ defmodule Livebook.Application do
})
end

# ELIXIR_ERL_OPTIONS used to configure EPMD module in releases.
defp config_env_var?("ELIXIR_ERL_OPTIONS"), do: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to delete this, otherwise any Elixir executable invoked within the runtime will have the custom EPMD configuration.

On the flip side, this means if the user has set ELIXIR_ERL_OPTIONS somewhere, it will no longer propagate to the runtime (like ERL_AFLAGS and ERL_ZFLAGS currently propagate). Is this acceptable?

Another option is to not delete ELIXIR_ERL_OPTIONS here, then it will be also available in the runtime (so we don't need to set it by hand), and then we delete it from the runtime instead.

So overall, we need to answer which of ERL_AFLAGS, ERL_ZFLAGS and ELIXIR_ERL_OPTIONS we want to pass all the way down, from Livebook -> Runtime -> Executable, or if we want to delete some (or all) of them along the way (and if so, where).

Copy link
Member

Choose a reason for hiding this comment

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

@josevalim we can set ELIXIR_ERL_OPTIONS_WAS or similar, and just revert the value here, so whatever the user sets gets propagated as expected, wdyt?

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 thought about that but I also think it is useful to have a value that only applies to Livebook but not the runtimes and ELIXIR_ERL_OPTIONS sounds like a good candidate. We just have to document it?

Copy link
Member

Choose a reason for hiding this comment

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

useful to have a value that only applies to Livebook but not the runtimes

We don't expect users to set it for Livebook specifically though? So if anything they want to set it for the runtime. Whatever you think is best :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't expect ERL_AFLAGS to be set it either, but it just works by the nature of things, it that makes any sense.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that Livebook-specific env var is probably not interesting for the end user, only us, so we can replicate that without affecting how envs work for the user. Again, no strong opinion though.

ensure_distribution!()
validate_hostname_resolution!()

if Livebook.Config.boolean!("LIVEBOOK_EPMDLESS", false) do
Copy link
Member

Choose a reason for hiding this comment

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

I think for consistency we should parse this in lib/livebook.ex and put in config.

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 was thinking about this... but the app env var is not really used anywhere else and I wanted to avoid us reading it (we should use Livebook.EPMD.dist_port instead to detect the status). Do you prefer the app env var anyway?

Copy link
Member

Choose a reason for hiding this comment

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

I would put it in config, even if used in this single place. I think we env -> config -> access almost everywhere and that makes it more centralized :)

Comment on lines 105 to 106
File.mkdir_p!("priv/epmd")
File.write!("priv/epmd/Elixir.Livebook.EPMD.beam", binary)
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in __after_compile__?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I will try :)

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.

I tested desktop app on both macOS and Windows and it works with and without epmd running!

I did notice one minor thing though: when epmd is not running and I start Livebook Desktop, then all of sudden epmd is running. Is that expected?

if [[ "$hostname" =~ " " ]]; then
echo "[error] system hostname ($hostname) cannot contain whitespaces"
exit 1
if [[ "$LIVEBOOK_EPMDLESS" != "false" && "$LIVEBOOK_EPMDLESS" != "0" ]]; then
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 we need to set the env var:

Suggested change
if [[ "$LIVEBOOK_EPMDLESS" != "false" && "$LIVEBOOK_EPMDLESS" != "0" ]]; then
export LIVEBOOK_EPMDLESS=${LIVEBOOK_EPMDLESS:-false}
if [[ "$LIVEBOOK_EPMDLESS" != "false" && "$LIVEBOOK_EPMDLESS" != "0" ]]; then

so that in Elixir land this can be executed:

if Livebook.Config.boolean!("LIVEBOOK_EPMDLESS", false) do

rel/server/env.bat.eex Show resolved Hide resolved
@josevalim josevalim merged commit 6d7f416 into main May 8, 2024
8 of 9 checks passed
@josevalim josevalim deleted the jv-epmdless branch May 8, 2024 08:05
@josevalim
Copy link
Contributor Author

💚 💙 💜 💛 ❤️

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