Implement :env_vars option for %Mix.Dep{} #6933

zorbash · 2017-10-15T18:29:54Z

This PR is the implementation of this proposal, which allows defining per dependency env variables to be used when loading or compiling it.

Example:

def deps do
  {:exometer, "~> 1.2.1", env_vars: [{"EXOMETER_PACKAGES", "(amqp)"}]}
end

Will set the EXOMETER_PACKAGES env variable to "(amqp)" which will include the amqp dependencies in the mix.lock (see: exometer-dependency-management). This is a Rebar specific example, but the PR sets the env vars to all types of supported packages.

All the files have been formatted using the new mix format task.
I'll be really happy to get feedback to improve this PR and make all the required changes to have it merged.

josevalim · 2017-10-15T19:42:36Z

lib/mix/lib/mix/dep/loader.ex

@@ -36,6 +36,15 @@ defmodule Mix.Dep.Loader do
    only != nil and env not in List.wrap(only)
  end

+  def with_env_vars(%Mix.Dep{env_vars: env_vars}, callback) do


Let's have a fast path clause that checks if env_vars is an empty list and then we just invoke the callback.

josevalim · 2017-10-15T19:42:46Z

lib/mix/lib/mix/dep/loader.ex

@@ -36,6 +36,15 @@ defmodule Mix.Dep.Loader do
    only != nil and env not in List.wrap(only)
  end

+  def with_env_vars(%Mix.Dep{env_vars: env_vars}, callback) do
+    env = System.get_env()
+    System.put_env(env_vars)


Let's use try/after here.

josevalim · 2017-10-15T19:43:19Z

lib/mix/lib/mix/tasks/deps.compile.ex

@@ -261,9 +261,15 @@ defmodule Mix.Tasks.Deps.Compile do
    end
  end

-  defp do_command(%Mix.Dep{app: app, opts: opts}, config, command, print_app?, env \\ []) do
+  defp do_command(
+         %Mix.Dep{app: app, env_vars: env_vars, opts: opts},


Please move %Mix.Dep{app: app, opts: opts} out of the function head and into the function body.

josevalim · 2017-10-15T19:43:44Z

lib/mix/test/mix/dep_test.exs

+
+    with_deps(
+      [
+        {


Please look at the other tests. You should define the deps as a variable before calling with_deps.

josevalim · 2017-10-15T19:44:21Z

/cc @mobileoverlord could this be useful to you?

josevalim · 2017-10-15T19:44:53Z

My only concern is regarding the name since "env" is a bit overloaded. I wonder if system_vars would or system_env would be a better name?

zorbash · 2017-10-15T21:44:36Z

If I had to choose between system_vars and system_env, I'd go with system_env as it's closer sounding to the System.*_env family of functions and might be more memorable.

ericmj · 2017-10-15T22:36:13Z

lib/mix/lib/mix/dep.ex

@@ -31,6 +31,8 @@ defmodule Mix.Dep do
      the information on this field is private to the manager and should not be
      relied on

+    * `env_vars` - an enumerable of tuples containing environment key-value as binary


This needs to be elaborated on. It doesn't say what the option does and when it's used. For example it is only used during loading and not during compilation.

I copied the text of the :env option of System.cmd/3 which is of the same type. You're absolutely right that it needs better doc there, I'll update it.

Should I also add an entry in the changelog about this?

We try to avoid updating the changelog in PRs because it easily leads to conflicts. An entry will be added before we do a new release of course.

Reworded it to:

an enumerable of key-value tuples of binaries to be set as environment variables when loading or compiling the dependency

I tried to keep keep it short to match the terseness of the rest of the options. Let me know what you think.

ericmj · 2017-10-15T22:38:08Z

Keep in mind that this option won't be honored for hex package dependencies so we will have to reject publishing to hex when the option is used.

josevalim · 2017-10-16T07:27:04Z

@ericmj do you have any preference regarding the name?

ericmj · 2017-10-16T07:38:12Z

I prefer system_env.

josevalim · 2017-10-16T07:39:31Z

@zorbash let's go with :system_env and please address the remaining comments. :)

zorbash · 2017-10-16T22:29:39Z

I've made changes reflecting suggestions in the comments and renamed :env_vars to :system_env.

@ericmj about #6933 (comment) Would it be desirable to submit a PR to hex to not publish when system_env is used?

ericmj · 2017-10-18T13:34:22Z

about #6933 (comment) Would it be desirable to submit a PR to hex to not publish when system_env is used?

Please do.

ericmj · 2017-10-18T13:35:28Z

lib/mix/lib/mix/dep/loader.ex

@@ -161,7 +174,8 @@ defmodule Mix.Dep.Loader do
      app: app,
      requirement: req,
      status: scm_status(scm, opts),
-      opts: Keyword.put_new(opts, :env, :prod)
+      opts: Keyword.put_new(opts, :env, :prod),
+      system_env: Keyword.get(opts, :system_env, [])


I don't think we have to separate this from opts.

It makes it easier to pattern match on it later and I don't think this needs to be available to the SCM, does it?

In any case, if we have a dedicated field for it, we should probably pop it from opts by calling Keyword.pop(opts, :system_env, []).

With system_env as a dedicated field, we can pattern match to reject a package from being published by adding:

%Mix.Dep{app: app, system_env: _} -> Mix.raise "Can't build package when :system_env is set for dependency #{app}, remove `system_env: ...`"

to https://github.com/hexpm/hex/blob/v0.17.1/lib/mix/tasks/hex.build.ex#L157

But I don't see how hex, containing such a change would keep being compatible with all the Elixir versions (other than master) defined in its .travis.yml, unless if we pattern match on it as a Map.

Hex wouldn't be able to pattern match on it, we would have to pull it out with Map.get/3. We don't seem to use it for pattern matching in this PR either.

@ericmj we pattern match on it both times we use it. :)

@ericmj if you're OK with something like:

Enum.each(include, fn %Mix.Dep{app: app, opts: opts} = dep -> if Map.get(dep, :system_env) do Mix.raise "Can't build package when :system_env is set for dependency #{app}, remove `system_env: ...`" end end)

we can keep the dedicated field and also use keyword.pop/3 as @josevalim suggested, so not to have it in the opts as it's not an SCM option.

@josevalim I meant pattern match in a conditional way, right now the pattern matches could be replaced with dep.system_env or opts[:system_env]. But it’s not a strong opinion so let’s go ahead with th dedicated field.

@zorbash yeah, something like that would work.

See: elixir-lang/elixir#6933 (comment)

ericmj · 2017-10-19T22:04:48Z

Thank you!

See: elixir-lang/elixir#6933 (comment)

josevalim reviewed Oct 15, 2017

View reviewed changes

ericmj reviewed Oct 15, 2017

View reviewed changes

zorbash added 4 commits October 16, 2017 23:04

Add :system_env option to %Mix.Dep{}

72bd8c3

Change Mix.Tasks.Deps.compile to pass :system_env to the shell

8e4926c

Add Mix.Dep.Loader.with_system_env/2

5db1305

Set env vars for dep upon load in Mix.Dep.Converger

730bafb

zorbash force-pushed the env-for-mix-deps-compile branch from c2519a6 to 730bafb Compare October 16, 2017 22:09

Fit dependency on a single line

63ceede

josevalim approved these changes Oct 17, 2017

View reviewed changes

ericmj reviewed Oct 18, 2017

View reviewed changes

Use Keyword.pop/3 to get :system_env in Mix.Dep.Loader

65008fc

zorbash added a commit to zorbash/hex that referenced this pull request Oct 19, 2017

Reject packages defining :system_env

01471f7

See: elixir-lang/elixir#6933 (comment)

zorbash mentioned this pull request Oct 19, 2017

Reject packages defining :system_env hexpm/hex#442

Merged

Remove unnecessary pipe

e8881f2

ericmj merged commit 055cff4 into elixir-lang:master Oct 19, 2017

zorbash deleted the env-for-mix-deps-compile branch October 19, 2017 22:10

ericmj pushed a commit to hexpm/hex that referenced this pull request Oct 19, 2017

Reject packages defining :system_env (#442)

9bb13f3

See: elixir-lang/elixir#6933 (comment)

gomoripeti mentioned this pull request Dec 19, 2017

Support both cowboy 1.x and 2.x version Appliscale/xprof#99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement :env_vars option for %Mix.Dep{} #6933

Implement :env_vars option for %Mix.Dep{} #6933

zorbash commented Oct 15, 2017

josevalim Oct 15, 2017

josevalim Oct 15, 2017

josevalim Oct 15, 2017

josevalim Oct 15, 2017

josevalim commented Oct 15, 2017

josevalim commented Oct 15, 2017

zorbash commented Oct 15, 2017

ericmj Oct 15, 2017 •

edited

Loading

zorbash Oct 16, 2017

ericmj Oct 16, 2017

zorbash Oct 16, 2017

ericmj commented Oct 15, 2017

josevalim commented Oct 16, 2017

ericmj commented Oct 16, 2017

josevalim commented Oct 16, 2017

zorbash commented Oct 16, 2017

ericmj commented Oct 18, 2017

ericmj Oct 18, 2017

josevalim Oct 18, 2017

zorbash Oct 18, 2017 •

edited

Loading

ericmj Oct 19, 2017

josevalim Oct 19, 2017

zorbash Oct 19, 2017

ericmj Oct 19, 2017

ericmj commented Oct 19, 2017

Implement :env_vars option for %Mix.Dep{} #6933

Implement :env_vars option for %Mix.Dep{} #6933

Conversation

zorbash commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Oct 15, 2017

josevalim commented Oct 15, 2017

zorbash commented Oct 15, 2017

ericmj Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmj commented Oct 15, 2017

josevalim commented Oct 16, 2017

ericmj commented Oct 16, 2017

josevalim commented Oct 16, 2017

zorbash commented Oct 16, 2017

ericmj commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorbash Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmj commented Oct 19, 2017

ericmj Oct 15, 2017 •

edited

Loading

zorbash Oct 18, 2017 •

edited

Loading