Add Mix.Tasks.Compile.reenable

[am-kantox]

Mix.Task.reenable/1 does not currently re-enable tasks which were implicitly invoked from Mix.Task.run/1.

For instance, when some task creates the new file within lib directory, the following code works for "compile.elixir" but results in {:noop, []} for "compile".

  File.write!("lib/z.ex", """
    defmodule Z do
      def ok, do: :ok
    end
  """)

  # fails because `"compile.elixir"` has not been re-enabled
  Mix.Task.reenable("compile")
  assert {:ok, []} = Mix.Tasks.Compile.run(["--verbose"])
  
  # works like a charm
  Mix.Task.reenable("compile.elixir")
  assert {:ok, []} = Mix.Tasks.Compile.Elixir.run(["--verbose"])

This PR introduces an optional callback depends_on/0 for both Mix.Task and Mix.Task.Compiler which is used to recursively re-enable tasks this one depends on.

[josevalim]

Can you please expand a bit more on the use case? Why are you reenabling the compiler? Also, if this is specific to the compiler, I would rather have a task for reenabling the compiler explicitly.

The reason I ask is because I can see someone wanting to reenable some compilers but not all of them, and this could be as well a change of behaviour. Also remember that you can always get the list of compilers and individually enable the ones you care about.

[am-kantox]

Can you please expand a bit more on the use case? Why are you reenabling the compiler?

I personally do reenable compiler because I have a task which generates a regular file from the template and then generates a test file based on the information which can be retrieved from the loaded module only. That said, before generating the test, I need the file to be compiled and loaded.

Also, if this is specific to the compiler, I would rather have a task for reenabling the compiler explicitly.

To my understanding, this is not specific to the compiler. I took it as an example, but the real issue is more generic.

AFAICT, Mix.Task.reenable("foo"); Mix.Task.run("foo") should be an exact equivalent of System.cmd("mix", ["foo"]), and it currently is not. Once run/1 invokes another run/1 under the hood, I would expect reenable/1 to reenable is from scratch, which apparently means reenabling both tasks (including the one implicitly called from the top-level run/1.)

The reason I ask is because I can see someone wanting to reenable some compilers but not all of them, and this could be as well a change of behaviour.

This is already possible with an explicit reenabling specific compilers. This PR is not after allowing a fine-tuning over what compilers to execute. It is fully about making reenable/1 to do what its name suggests, rolling back the tasks “state” to make a subsequent call to run/1 do exactly the same as the previous one did. Now reenable(:compile) is next to noop, unless reenable("compile.all") has been also invoked.

Also remember that you can always get the list of compilers and individually enable the ones you care about.

As I hopefully clarified above, I understand this (also the example in the PR comment shows that,) but this PR is not about that. I am after the ability to make it possible to roll back my own task "foo" which invokes another task "bar" from its run/1 by Task.reenable("foo"). Currently that’s not the case. The consumer of this task must read the code and then do Task.reenable("bar"); Task.reenable("foo").

[josevalim]

Thank you. If the compiler is not your use case, then I would like to hear a more concrete example. The ultimate issue is that the concept of reenable one and reenable all are both equally flawed. mix test calls mix compile but I may still want to re-run tests without checking the compiler.

If your goal is to make it behave as if they were separate CLI invocations, there is already Mix.Task.clear, and that's arguably the correct function for what you want. After all, most tasks are useless without calling either one of "app.start", "app.load" or "compile", and I would expect your proposed reenable semantics to reenable them as well.

[am-kantox]

If the compiler is not your use case, then I would like to hear a more concrete example.

Well, the compiler is indeed my use-case, but I have a habit to read sources if something goes in any unexpected way, which is unfortunately not what 100% of developers would do in the first place. I must have expressed myself poorly, let me try again with another example below.

The ultimate issue is that the concept of reenable one and reenable all are both equally flawed. mix test calls mix compile but I may still want to re-run tests without checking the compiler.

That is exactly why I didn’t propose gathering this info from the task source code and/or do it in any other automatical fashion. The particular task author would surely know if any other task is to be unlimately re-enabled within their own task. "test" does not implement depends_on/0 for this exact reason (should I have proposed reenable_with/0 as the name for this callback for clarity?

If your goal is to make it behave as if they were separate CLI invocations, there is already Mix.Task.clear, and that's arguably the correct function for what you want.

Well, it would work, but I am trying to introduce a fine tuning. Please see an example below.

After all, most tasks are useless without calling either one of "app.start", "app.load" or "compile", and I would expect your proposed reenable semantics to reenable them as well.

My proposed semantics does not imply any rules. It’s totally up to the task author what to reenable within their task. As you mentioned above, mix test is often wanted to be re-enabled without compilation, that’s why I didn’t propose to amend it. The last but not the least argument is that when a developer re-enables "test", they would not expect the whole clear to be called. But "compile" is different from the user’s point of view. I was really surprised that when I call reenable("compile") nothing really happens, when I call reenable("compile"); reenable("compile.elixir") (followed by run("compile")) nothing happens either, and what I actually need to call is reenable("compile"); reenable("compile.all"). This cannot be guessed, only grasped from the source code.

Example

defmodule Mix.Tasks.My.Config do
  use Mix.Task

  @impl Mix.Task
  def run([file]) do
    File.write!(file, …)
  end
end

defmodule Mix.Tasks.My.Test do
  use Mix.Task

  @impl Mix.Task
  def run(args) do
    {test, config} = args |> OptionParser.parse(…) |> reshape()

    Mix.Task.run("my.config", [config])
    Mix.Task.run("test", [test])
  end
end

defmodule Mix.Tasks.My.Tests do
  use Mix.Task

  @impl Mix.Task
  def run(args) do
    [{_test, _config} | _] = 
      test_configs = 
      args |> OptionParser.parse(…) |> reshape()

    Enum.each(test_configs, fn {test, config} ->
      # reenabling only this won’t work as expected
      # "my.config" won’t be re-enabled
      Mix.Task.reenable("my.test") 
      
      Mix.Task.run("my.test", ["--test", test, "--config", config])
    end)
  end
end

The code above might be made more user-friendly by

 defmodule Mix.Tasks.My.Test do
   use Mix.Task

   @impl Mix.Task
   def run(args) do
     {test, config} = args |> OptionParser.parse(…) |> reshape()

     Mix.Task.run("my.config", [config])
     Mix.Task.run("test", [test])
   end

+  @impl Mix.Task
+  def depends_on do
+    ["my.config", "test"]
+  end
 end

This could be especially helpful if both "my.test" and "my.config" are from some external library and I am the author of "my.tests" only. The library author kinda helps me to not go to the source code to figure out what am I to re-enable to make my task work. THe code might explicitly tell mix, “hey, when I am re-enabled, please also re-enable this and that, I know what I am doing here.”

---

If the above does not convince you either, feel free to close the PR, because it seems I am the only one who entered into the necessity to dig the source code to figure out what to re-enable besides the task I really need to re-enable.

[josevalim]

My proposed semantics does not imply any rules. It’s totally up to the task author what to reenable within their task.

Yes, this is the concern in my first comment. It depends on the author and I am not sure if the default implementation of Mix.Task.reenable("compile") should automatically reenable all child tasks. We can definitely agree on reenabling "compile.all", but not the others. So unless we say we have to "reenable all", which is not necessarily useful, we have arbitrary rules, which are not necessarily useful either.

---

Since your ultimate goal is to reenable the compiler, we can expose something Mix.Task.Compiler.reenable_all with some options and customizations, so users can call it. Per the above, I don't think "depends on" itself is a useful abstraction, based on the examples so far. Thanks!

[am-kantox]

OK, so I am to change this PR to:

• reenable compile.all upon reenabling compile, which would make it possible to explicitly select compilers to reenable as Mix.Task.reenable("compile.elixir"); Mix.Task.rerun("compile") which looks semantically perfect,
• expose Mix.Task.Compiler.reenable(compilers: [compiler()]) so that we can start looking into “some options and customizations” from there.

Right?

[josevalim]

Just the second part. Add Mix.Task.Compiler.reenable(compilers: [compiler()]). Which default do we have for compilers? All or none?

[am-kantox]

None is already covered, this task is in Mix. Task.Compiler namespace, I vote for all.

[josevalim]

Sorry, none would not include compile.all. compile.all is always included, but you can choose the compilers. And FWIW, the Phoenix compiler does this: https://github.com/phoenixframework/phoenix/blob/main/lib/phoenix/code_reloader/server.ex#L247

[am-kantox]

BTW, maybe reenable(compilers :: [compiler], opts :: keyword) would make more sense? The namespace is already Compiler, I think plain list as the first argument is more handy.

[josevalim]

No need to check for the arguments.

[josevalim]

Let's go with all by default, because it is easier to customize.

Suggested change

	case Keyword.get(opts, :compilers, []) do
	:all -> Mix.Tasks.Compile.compilers()
	list when is_list(list) -> list
	end
	Keyword.get_lazy(opts, :compilers, &Mix.Tasks.Compile.compilers/0)

I am also thinking we should move this function to Mix.Tasks.Compile, since compilers is already defined there.

[josevalim]

I am unsure if we should enable loadpaths. Thoughts? Why not deps.loadpathsas well? That's one of the slippery slopes that worry me. :)

[am-kantox]

I am unsure if we should enable loadpaths.

We should not. It is a leftover from the code when I thought the enabling should be mirrored by a roll-back, but it should not in the latest approach. I cannot think of any change resulting in a need to reload paths from within same external task execution, even if it calls reenable/1.

[josevalim]

Please remove the changes to this function. :)

[josevalim]

There is one question remaining, which is about protocols consolidation. It is treated as a separate compiler, but it should most likely be reenabled too. There are a few things to consider:

• Phoenix reenables consolidation
• However, there is currently an issue in Phoenix: Protocols no longer consolidated after CodeReloader executes phoenixframework/phoenix#5831 - and I would like to investigate it before
• The type system will most likely require us to move consolidation into the Elixir compiler, which will by default affect any reenable semantics. Perhaps we just always reenable consolidation if the :elixir compile is given, and that's it

[am-kantox]

protocols consolidation

I believe, this issue has nothing to do with Phoenix per se.

compile.protocols_test.exs is kinda flawed because it inevitably calls tasks "compile.elixir" and "compile.protocols", while "compile.protocols" calls "compile" which (as we can see above) does not invoke "compile.elixir" via "compile.all" unless the latter has been explicitly re-enabled.

I am to start with changing tests to fail by changing direct calls to Compiler.run/1 to Task.run("compile.compiler") and adding assertions for Compile.Protocol.__protocol__(:impls), then we’ll see how to handle it from there.

we just always reenable consolidation if the :elixir compile is given

That would be most likely the best resulting code of this PR, but first I want to make sure re-enabling protocols works as expected within current design (separated task for protocols.)

[josevalim]

Yeah. I mean the Phoenix PR is related to this one because Phoenix reenables the compilers, and I would be nice if Phoenix could use this API as well. If it can, it is a good indicator this is general, if not, it may be too specific. But in order to assess this, we need to fix current bugs.

[am-kantox]

The plot thickens.

compile.protocols is a task, but it is not a “regular” compiler by any means. The code which tries to extract it from compilers: option quickly became unreadable, also it’s unclear how to handle contradictory compilers lists given.

The cleanest approach I could come with, would be to enforce elixir compiler when compile.protocols was requested and introduce another argument to Mix.Tasks.Compile.reenable/1 protocols: :consolidate. Once given, this option implies "compile.elixir" to be reenabled as well.

The phoenix code could with this change to be simplified to

  defp mix_compile(compilers, compile_args, config, consolidation_path) do
    compilers = config[:compilers] || Mix.compilers()
    protocols = if config[:consolidate_protocols], do: :consolidate
    Mix.Task.Compile.reenable(compilers: compilers, protocols: protocols)

    # We call build_structure mostly for Windows so new
    # assets in priv are copied to the build directory.
    Mix.Project.build_structure(config)
    args = ["--purge-consolidation-path-if-stale", consolidation_path | compile_args]

    result =
      with_logger_app(config, fn ->
        Mix.Task.run("compile", args)
      end)

    with :error <- result, do: exit({:shutdown, 1})
  end

---

I receive the following error while running the complete test suite, but I am unsure if it’s related because no changed in this PR code could somehow affect the behaviour due to my best assessment:

  1) test runs in daemon mode (Mix.Tasks.ReleaseTest)
     /home/am/Proyectos/Elixir/elixir/lib/mix/test/mix/tasks/release_test.exs:716
     ** (ExUnit.TimeoutError) test timed out after 60000ms. You can change the timeout:

       1. per test by setting "@tag timeout: x" (accepts :infinity)
       2. per test module by setting "@moduletag timeout: x" (accepts :infinity)
       3. globally via "ExUnit.start(timeout: x)" configuration
       4. by running "mix test --timeout x" which sets timeout
       5. or by running "mix test --trace" which sets timeout to infinity
          (useful when using IEx.pry/0)

     where "x" is the timeout given as integer in milliseconds (defaults to 60_000).
     
     stacktrace:
       (elixir 1.18.0-dev) lib/process.ex:314: Process.sleep/1
       test/mix/tasks/release_test.exs:807: Mix.Tasks.ReleaseTest.wait_until/1
       test/mix/tasks/release_test.exs:743: anonymous fn/1 in Mix.Tasks.ReleaseTest."test runs in daemon mode"/1
       (mix 1.18.0-dev) lib/mix/project.ex:463: Mix.Project.in_project/4
       (elixir 1.18.0-dev) lib/file.ex:1665: File.cd!/2
       test/test_helper.exs:156: MixTest.Case.in_fixture/3
       test/mix/tasks/release_test.exs:717: (test)
       (ex_unit 1.18.0-dev) lib/ex_unit/runner.ex:495: ExUnit.Runner.exec_test/2
       (stdlib 6.0.1) timer.erl:590: :timer.tc/2
       (ex_unit 1.18.0-dev) lib/ex_unit/runner.ex:417: anonymous fn/6 in ExUnit.Runner.spawn_test_monitor/4

[am-kantox]

If :elixir has been given, append :protocols right after it, prepend both :elixir and :protocols to :app otherwise, so that :app is still the latest compiler.

I am not sure if this is actually needed.

[am-kantox]

:ok here and below is because :protocols now implies :elixir invocation which would return {:ok, []} not {:noop, []}. Checks for Compile.Protocol.__protocol__(:impls) now plays the role of real checker.

[josevalim]

And this is going to change soon, because protocols will be bundled into the Elixir compiler. Perhaps we should prioritize that work to be on 1.18 and we can merge this with more confidence. Stay tuned!

[josevalim]

Apologies, this will be postponed to v1.19 since I was not able to get the compile.protocols refactor in time for v1.18.

[am-kantox]

Apologies, this will be postponed to v1.19 since I was not able to get the compile.protocols refactor in time for v1.18.

No issue. Would you like me to try to help with compile.protocols?

I understand at least 20% of the issues with it since I dug into this dependency hell when I first came out with this proposal. I might be missing a ton of internals, though, so I would be fine with “your approach sucks” as a result :)

[josevalim]

No need, thanks, since I still need to figure out how exactly it will look like.

[am-kantox]

No need, thanks, since I still need to figure out how exactly it will look like.

Sometimes the look from the other bench helps.

OK, just shout out when you have run out of ideas.