Rely on mtime to recompile app (#14012)

Author: josevalim

lib/mix/lib/mix/release.ex

@@ -882,9 +882,7 @@ defmodule Mix.Release do
 
   defp process_app_file(source_file, target_file) do
     with {:ok, [{:application, app, info}]} <- :file.consult(source_file) do
-      # Remove :config_mtime so that .app files are deterministic between builds
-      new_info = Keyword.delete(info, :config_mtime)
-      File.write!(target_file, :io_lib.format("~tp.~n", [{:application, app, new_info}]))
+      File.write!(target_file, :io_lib.format("~tp.~n", [{:application, app, info}]))
     else
       _ -> File.cp!(source_file, target_file)
     end

lib/mix/lib/mix/tasks/compile.app.ex

@@ -92,6 +92,13 @@ defmodule Mix.Tasks.Compile.App do
     * `--compile-path` - where to find `.beam` files and write the
       resulting `.app` file, defaults to `Mix.Project.compile_path/0`
 
+  ## Configuration
+
+    * `:reliable_dir_mtime` - this task relies on the operating system
+      changing the mtime on a directory whenever a file is added or removed.
+      You can set this option to false if your system does not provide
+      reliable mtimes. Defaults to false on Windows.
+
   ## Phases
 
   Applications provide a start phases mechanism which will be called,
@@ -130,50 +137,58 @@ defmodule Mix.Tasks.Compile.App do
   @impl true
   def run(args) do
     {opts, _, _} = OptionParser.parse(args, switches: [force: :boolean, compile_path: :string])
-
     project = Mix.Project.get!()
     config = Mix.Project.config()
 
     app = Keyword.get(config, :app)
     version = Keyword.get(config, :version)
+    validate_app!(app)
+    validate_version!(version)
 
-    validate_app(app)
-    validate_version(version)
+    compile_path = Keyword.get_lazy(opts, :compile_path, &Mix.Project.compile_path/0)
+    target = Path.join(compile_path, "#{app}.app")
 
-    path = Keyword.get_lazy(opts, :compile_path, &Mix.Project.compile_path/0)
-    modules = modules_from(path) |> Enum.sort()
-
-    target = Path.join(path, "#{app}.app")
-
-    # We mostly depend on the project_file through the def application function,
-    # but it doesn't hurt to also include config_mtime.
+    # If configurations changed, we may have changed compile_env.
+    # If compile_path changed, we may have added or removed files.
+    # If the project changed, we may have changed other properties.
     new_mtime =
-      max(Mix.Project.config_mtime(), Mix.Utils.last_modified(Mix.Project.project_file()))
+      Mix.Project.config_mtime()
+      |> max(Mix.Utils.last_modified(Mix.Project.project_file()))
+      |> max(Mix.Utils.last_modified(compile_path))
 
     current_properties = current_app_properties(target)
-    compile_env = load_compile_env(current_properties)
-    old_mtime = Keyword.get(current_properties, :config_mtime, 0)
 
-    if opts[:force] || new_mtime > old_mtime ||
-         app_changed?(current_properties, modules, compile_env) do
+    {changed?, modules} =
+      cond do
+        opts[:force] || new_mtime > Mix.Utils.last_modified(target) ->
+          {true, nil}
+
+        Keyword.get(config, :reliable_dir_mtime, fn -> not match?({:win32, _}, :os.type()) end) ->
+          {false, nil}
+
+        true ->
+          modules = modules_from(compile_path)
+          {modules != Keyword.get(current_properties, :modules, []), modules}
+      end
+
+    if changed? do
       properties =
         [
           description: to_charlist(config[:description] || app),
-          modules: modules,
           registered: [],
           vsn: to_charlist(version)
         ]
         |> merge_project_application(project)
-        |> validate_properties!()
         |> handle_extra_applications(config)
-        |> add_compile_env(compile_env)
+        |> add_compile_env(current_properties)
+        |> add_modules(modules, compile_path)
 
-      properties = [config_mtime: new_mtime] ++ properties
       contents = :io_lib.format("~p.~n", [{:application, app, properties}])
       :application.load({:application, app, properties})
 
       Mix.Project.ensure_structure()
       File.write!(target, IO.chardata_to_string(contents))
+      File.touch!(target, new_mtime)
       Mix.shell().info("Generated #{app} app")
       {:ok, []}
     else
@@ -189,27 +204,15 @@ defmodule Mix.Tasks.Compile.App do
     end
   end
 
-  defp load_compile_env(current_properties) do
-    case Mix.ProjectStack.compile_env(nil) do
-      nil -> Keyword.get(current_properties, :compile_env, [])
-      list -> list
-    end
-  end
-
-  defp app_changed?(properties, mods, compile_env) do
-    Keyword.get(properties, :modules, []) != mods or
-      Keyword.get(properties, :compile_env, []) != compile_env
-  end
+  defp validate_app!(app) when is_atom(app), do: :ok
 
-  defp validate_app(app) when is_atom(app), do: :ok
-
-  defp validate_app(app) do
-    ensure_present(:app, app)
+  defp validate_app!(app) do
+    ensure_present!(:app, app)
     Mix.raise("Expected :app to be an atom, got: #{inspect(app)}")
   end
 
-  defp validate_version(version) do
-    ensure_present(:version, version)
+  defp validate_version!(version) do
+    ensure_present!(:version, version)
 
     if not (is_binary(version) and match?({:ok, _}, Version.parse(version))) do
       Mix.raise(
@@ -218,18 +221,20 @@ defmodule Mix.Tasks.Compile.App do
     end
   end
 
-  defp ensure_present(name, nil) do
+  defp ensure_present!(name, nil) do
     Mix.raise("Please ensure mix.exs file has the #{inspect(name)} in the project definition")
   end
 
-  defp ensure_present(_name, _val), do: :ok
+  defp ensure_present!(_name, _val), do: :ok
 
   defp modules_from(path) do
     case File.ls(path) do
       {:ok, entries} ->
-        for entry <- entries,
-            String.ends_with?(entry, ".beam"),
-            do: entry |> binary_part(0, byte_size(entry) - 5) |> String.to_atom()
+        Enum.sort(
+          for entry <- entries,
+              String.ends_with?(entry, ".beam"),
+              do: entry |> binary_part(0, byte_size(entry) - 5) |> String.to_atom()
+        )
 
       {:error, _} ->
         []
@@ -246,7 +251,7 @@ defmodule Mix.Tasks.Compile.App do
         )
       end
 
-      Keyword.merge(best_guess, project_application)
+      Keyword.merge(best_guess, validate_properties!(project_application))
     else
       best_guess
     end
@@ -357,8 +362,21 @@ defmodule Mix.Tasks.Compile.App do
   defp typed_app?({app, type}) when is_atom(app) and type in [:required, :optional], do: true
   defp typed_app?(_), do: false
 
-  defp add_compile_env(properties, []), do: properties
-  defp add_compile_env(properties, compile_env), do: [compile_env: compile_env] ++ properties
+  defp add_compile_env(properties, current_properties) do
+    # If someone calls compile.elixir and then compile.app across two
+    # separate OS calls, then the compile_env won't be properly reflected.
+    # This is ok because compile_env is not used for correctness. It is
+    # simply to catch possible errors early.
+    case Mix.ProjectStack.compile_env(nil) do
+      nil -> Keyword.take(current_properties, [:compile_env]) ++ properties
+      [] -> properties
+      compile_env -> Keyword.put(properties, :compile_env, compile_env)
+    end
+  end
+
+  defp add_modules(properties, modules, compile_path) do
+    Keyword.put_new_lazy(properties, :modules, fn -> modules || modules_from(compile_path) end)
+  end
 
   defp handle_extra_applications(properties, config) do
     {extra, properties} = Keyword.pop(properties, :extra_applications, [])

lib/mix/test/mix/release_test.exs

@@ -716,13 +716,6 @@ defmodule Mix.ReleaseTest do
 
       assert mode!(source_so_path) == mode!(tmp_path("mix_release/libtest_nif.so"))
     end
-
-    test "removes config_mtime from app files" do
-      assert copy_ebin(release([]), @eex_ebin, tmp_path("eex_ebin"))
-
-      {:ok, [{:application, :eex, info}]} = :file.consult(tmp_path("eex_ebin/eex.app"))
-      refute Keyword.get(info, :config_mtime)
-    end
   end
 
   describe "copy_app/2" do
@@ -755,14 +748,6 @@ defmodule Mix.ReleaseTest do
       refute File.exists?(Path.join(@release_lib, "runtime_tools-#{@runtime_tools_version}/ebin"))
       refute File.exists?(Path.join(@release_lib, "runtime_tools-#{@runtime_tools_version}/priv"))
     end
-
-    test "removes config_mtime from app files" do
-      assert copy_app(release(strip_beams: false, applications: [eex: :permanent]), :eex)
-
-      eex_app_path = Path.join(@release_lib, "eex-#{@elixir_version}/ebin/eex.app")
-      {:ok, [{:application, :eex, info}]} = :file.consult(eex_app_path)
-      refute Keyword.get(info, :config_mtime)
-    end
   end
 
   describe "strip_beam/1" do

lib/mix/test/mix/tasks/compile.app_test.exs

@@ -86,24 +86,32 @@ defmodule Mix.Tasks.Compile.AppTest do
   test "generates .app file with compile_env" do
     in_fixture("no_mixfile", fn ->
       Mix.Project.push(MixTest.Case.Sample)
+      File.mkdir_p!("config")
+      File.write!("config/config.exs", "[]")
+      Mix.Task.run("loadconfig")
+
+      reset_config = fn ->
+        Mix.ProjectStack.reset_config_mtime()
+        ensure_touched("config/config.exs", "_build/dev/lib/sample/ebin/sample.app")
+      end
 
       Mix.ProjectStack.compile_env([{:app, :key, :error}])
       assert Mix.Tasks.Compile.App.run([]) == {:ok, []}
       assert parse_resource_file(:sample)[:compile_env] == [{:app, :key, :error}]
 
       # No-op with untouched unset compile_env
-      assert Mix.Tasks.Compile.App.run([]) == {:noop, []}
-
-      # No-op with same compile_env
-      Mix.ProjectStack.compile_env([{:app, :key, :error}])
-      assert Mix.Tasks.Compile.App.run([]) == {:noop, []}
+      reset_config.()
+      assert Mix.Tasks.Compile.App.run([]) == {:ok, []}
+      assert parse_resource_file(:sample)[:compile_env] == [{:app, :key, :error}]
 
       # Recompiles with new compile_env
+      reset_config.()
       Mix.ProjectStack.compile_env([{:app, :another, :error}])
       assert Mix.Tasks.Compile.App.run([]) == {:ok, []}
       assert parse_resource_file(:sample)[:compile_env] == [{:app, :another, :error}]
 
       # Keeps compile_env if forcing
+      reset_config.()
       assert Mix.Tasks.Compile.App.run(["--force"]) == {:ok, []}
       assert parse_resource_file(:sample)[:compile_env] == [{:app, :another, :error}]
     end)