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

Use :extra_applications instead of :applications in escript.build #5534

Merged
merged 2 commits into from Dec 5, 2016
Merged

Use :extra_applications instead of :applications in escript.build #5534

merged 2 commits into from Dec 5, 2016

Conversation

whatyouhide
Copy link
Member

I wonder, could this veeery simple change be a solution for #5529? 🤔 :)

@josevalim
Copy link
Member

josevalim commented Dec 4, 2016 via email

@josevalim
Copy link
Member

josevalim commented Dec 4, 2016 via email

@whatyouhide
Copy link
Member Author

@josevalim I implemented both suggestions. While changing the test related to this in particular:

  defmodule EscriptErlangWithDeps do
    def project do
      [app: :escripttesterlangwithdeps,
       version: "0.0.1",
       language: :erlang,
       escript: [main_module: :escripttest],
       deps: [{:ok, path: fixture_path("deps_status/deps/ok")}]]
    end

    def application do
      [applications: []]
    end
  end

I left :applications to [] since we don't want to bring the :ok application in the escript as that would require to embed Elixir. Out of curiosity, I tried using applications: [:ok] and escript: [embed_elixir: true] and it in fact worked, but then I tried applications: [:ok], extra_applications: [:elixir] (with embed_elixir: false) and it didn't work, failing to load the :elixir application; is this expected and I'm missing something? I know about the docs of escript.build that say

you will have to add paths to Elixir’s ebin directories to ERL_LIBS environment variable when running the resulting escript, in order for the code loader to be able to find :elixir application and its children applications (if they are used).

but I expected that putting :elixir in the :extra_applications would do the trick anyways, but here we don't do anything about extra applications for Erlang escripts.

Sorry for the convoluted wall of text, hope my questions are clear enough 😄

@josevalim
Copy link
Member

but I expected that putting :elixir in the :extra_applications would do the trick anyways,

No because we change the escript generated based on the language configuration. Basically, you never add :elixir to :extra_applications, Mix takes care of it for you.

@josevalim
Copy link
Member

I guess though you found a bug in that if someone depends on logger and not the parent project, :logger won't be included after all. I think we should open up a separete report for it though, it is not related to this bug per se, as it likely exists since the beginning of escript.build (and this one is a regression).

@whatyouhide
Copy link
Member Author

@josevalim ok, got it. I will investigate and open another bug for that. Regarding this PR, do we need to change anything else?

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Looks good to me! Please backport to v1.4!

@whatyouhide whatyouhide merged commit bf5882b into elixir-lang:master Dec 5, 2016
@whatyouhide whatyouhide deleted the extra-applications-in-escripts branch December 5, 2016 12:47
@whatyouhide
Copy link
Member Author

Awesome, backported to v1.4 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants