Merge pull request #11 from ausimian/issue/8-stale-appup

fix: stop the appup compiler leaving a stale artefact in ebin

Author: ausimian

README.md

@@ -160,7 +160,19 @@ for you. The steps are as follows:
      ]
    end
    ```
-   
+
+The compiler owns `<app>.appup` for the whole of its life. It rewrites it on every
+build, and deletes it again if the source goes away or the `:appup` key is dropped,
+so that an incremental build cannot ship upgrade instructions belonging to an
+earlier version. Naming a file that does not exist is a compilation error: the
+project asked for an appup and cannot have one.
+
+That housekeeping only happens while the compiler is registered. To turn an appup
+off for some environments, leave `:appup` in `:compilers` and let the `:appup` key
+be `nil` - the output is removed and nothing further is reported. Taking the
+compiler out of `:compilers` instead stops it running, and an output an earlier
+build wrote stays where it is.
+
 ## Relup Generation
 
 Forecastle contains a mix task, `forecastle.relup`, that simplifies the generation of

RELEASE.md

@@ -15,6 +15,22 @@
   will not boot. Configuration is expanded at boot by the `env.sh`
   integration, which has no `env.bat` counterpart. This has always been the
   case; it was previously silent.
+- **Breaking:** the `:appup` compiler now fails the build when the `:appup`
+  project key names a file that does not exist, rather than warning and
+  carrying on. The project asked for an appup and cannot have one, and the
+  alternative is a release whose missing upgrade instructions only surface
+  later — in `:systools.make_relup/4`, or during the upgrade itself. Its
+  messages also reach the shell now: diagnostics returned by a compiler are
+  for editors to display inline, and nothing prints them on the command line.
+  A project that only has an appup in some environments should say so, rather
+  than name a file that is not there:
+
+  ```elixir
+  appup: if(Mix.env() == :prod, do: "appup.exs")
+  ```
+
+  A `nil` key is the supported off switch: it removes any output an earlier
+  build left and reports nothing further.
 - **Breaking:** the standard Mix launcher, `bin/<release>`, is no longer
   replaced. It keeps everything Mix gives it — cookie handling, distribution,
   `eval`/`rpc`/`remote`, daemon mode, version selection — and stays current with
@@ -57,6 +73,32 @@
   to manage its own releases.
 - The `GitHub` link in the Hex package metadata pointed at the Castle
   repository rather than Forecastle's.
+- The `:appup` compiler left `<app>.appup` behind in `ebin` once the project
+  stopped asking for one, whether because the source file was deleted or
+  because the `:appup` key was removed. It only worked out where the output
+  went on its way to writing it, so neither of those cases could remove
+  anything. An incremental build — which is what a CI cache produces —
+  therefore went on packaging upgrade instructions from an earlier version of
+  the application, and `release_handler` applied that obsolete plan during a
+  hot upgrade. The stale output is now deleted instead. Leaving the `:appup`
+  key unset is a supported way to turn an appup off for an environment: the
+  earlier output is removed, and beyond saying so once, nothing is reported.
+  Removal needs the compiler to stay in `:compilers` — dropping it from the
+  list stops it running at all, as it would any Mix compiler.
+- The `:appup` project key is resolved relative to the project file, as the
+  README has always said it is, rather than to whatever the working directory
+  happens to be. That is what makes "the source is missing" a trustworthy
+  verdict, now that it deletes the output and fails the build.
+- The `:appup` compiler returned a bare diagnostic where `Mix.Task.Compiler`
+  expects a list of them, so Mix discarded it and reported that the compiler
+  had misbehaved instead of saying what was wrong. It also ignored the result
+  of writing the appup, and so reported success when the write had failed.
+- The appup was written as the formatter produced it, which is Unicode
+  chardata rather than iodata. An appup containing a codepoint above 255 —
+  a module or term with a non-ASCII name — failed to write at all, and one
+  between 128 and 255 was written as a lone byte that `:file.consult/1`
+  cannot read back, so the build reported success and left behind an appup
+  that `systools` will not parse. It is encoded as UTF-8 now.
 
 ### Upgrading an existing deployment
 

lib/mix/tasks/compile/appup.ex

@@ -1,6 +1,26 @@
 defmodule Mix.Tasks.Compile.Appup do
   @moduledoc """
   Compiles appup files into the application's ebin folder.
+
+  The `:appup` project key names a file, relative to the project file, that is
+  evaluated for its value. It must not introduce top-level bindings. Whatever it
+  returns is written to `<app>.appup` alongside the application's beams.
+
+  The output is removed again whenever the project stops asking for it, either
+  because the source is gone or because the `:appup` key was dropped. Leaving it
+  in place would let an incremental build - which is what a CI cache produces -
+  ship upgrade instructions from an earlier version of the application, and
+  `release_handler` would then apply that obsolete plan during a hot upgrade.
+
+  A configured but missing source is a compilation error: the project asked for
+  an appup and cannot have one, and the alternative is a release that only fails
+  later, in `:systools.make_relup/4` or during the upgrade itself.
+
+  Removal only happens while the compiler is registered. Taking `:appup` out of
+  `:compilers` stops it running at all, and whatever an earlier build wrote then
+  stays where it is - as it would for any Mix compiler dropped from the list. To
+  turn an appup off per environment, leave the compiler registered and let the
+  `:appup` key be `nil`: that path removes the output and says nothing further.
   """
   @shortdoc "Compiles appup files"
   use Mix.Task.Compiler
@@ -9,23 +29,128 @@ defmodule Mix.Tasks.Compile.Appup do
 
   @impl true
   def run(_args) do
-    if src = Mix.Project.config()[:appup] do
-      if File.exists?(src) do
-        {appup, []} = Code.eval_file(src)
-        dst = Path.join(Mix.Project.compile_path(), "#{Mix.Project.config()[:app]}.appup")
-        File.write(dst, :io_lib.format(~c"~tp.~n", [appup]))
-      else
-        {:ok, diagnostic(:warning, "Appup file not found: #{src}")}
-      end
+    dst = destination()
+
+    case source() do
+      nil -> discard(dst)
+      src -> build(src, dst)
+    end
+  end
+
+  # `mix clean` also removes the application's whole build directory, so the
+  # appup goes with it either way. This is the callback the behaviour asks any
+  # compiler that writes output to define, and it keeps removing the artefact
+  # the responsibility of the task that created it.
+  @impl true
+  def clean do
+    _ = File.rm(destination())
+    :ok
+  end
+
+  # No manifests/0: the appup is regenerated on every build, so its timestamp
+  # always advances, and advertising it as a manifest would tell everything that
+  # consults `Mix.Task.Compiler.manifests/1` that the build changed every time.
+  # Regenerating unconditionally is deliberate - the source is arbitrary code
+  # whose result need not be a function of the source's own mtime.
+
+  defp build(src, dst) do
+    if File.exists?(src) do
+      write(src, dst)
     else
-      {:ok, diagnostic(:warning, "No appup specified in project")}
+      discard(dst, report(:error, "Appup file not found: #{src}"))
+    end
+  end
+
+  defp write(src, dst) do
+    {appup, []} = Code.eval_file(src)
+
+    case encode(appup) do
+      bytes when is_binary(bytes) -> put(dst, bytes)
+      _not_encodable -> {:error, report(:error, "Could not encode the appup in #{src} as UTF-8")}
     end
   end
 
-  defp diagnostic(severity, message, file \\ Mix.Project.project_file()) do
+  # `:io_lib.format` with `~tp` returns Unicode chardata, not iodata. A codepoint
+  # above 255 makes `File.write/2` fail outright; one between 128 and 255 is
+  # written as a lone byte that `:file.consult/1` - which reads UTF-8 - cannot
+  # read back, so the build reports success and ships an appup that `systools`
+  # will not parse. An appup that is not what the build thinks it is happens to
+  # be the whole failure this compiler exists to prevent.
+  defp encode(appup), do: :unicode.characters_to_binary(:io_lib.format(~c"~tp.~n", [appup]))
+
+  defp put(dst, bytes) do
+    case File.write(dst, bytes) do
+      :ok ->
+        :ok
+
+      {:error, reason} ->
+        {:error, report(:error, "Could not write #{dst}: #{:file.format_error(reason)}")}
+    end
+  end
+
+  # The project has opted out: `:appup` is unset. That is a legitimate state and
+  # not something to report on every compile, so say nothing unless there is an
+  # earlier build's output to take away - which happens on exactly one build.
+  defp discard(dst) do
+    case remove(dst) do
+      :removed -> {:ok, report(:information, "Removed #{dst}: no appup is specified")}
+      :absent -> :noop
+      {:error, diagnostics} -> {:error, diagnostics}
+    end
+  end
+
+  # The project asked for an appup and cannot have one. Take the previous one
+  # away regardless - leaving it behind is the whole bug - and fail with the
+  # diagnostic that explains why.
+  defp discard(dst, diagnostics) do
+    case remove(dst) do
+      {:error, extra} -> {:error, diagnostics ++ extra}
+      _removed_or_absent -> {:error, diagnostics}
+    end
+  end
+
+  # Failing to remove the output is a compilation error in its own right: a
+  # stale appup that could not be deleted is the bug still present.
+  defp remove(dst) do
+    case File.rm(dst) do
+      :ok ->
+        :removed
+
+      {:error, :enoent} ->
+        :absent
+
+      {:error, reason} ->
+        {:error, report(:error, "Could not remove #{dst}: #{:file.format_error(reason)}")}
+    end
+  end
+
+  # Relative to the project file rather than the working directory. The compiler
+  # is recursive, so Mix runs it from each umbrella child's own directory, but
+  # nothing guarantees the working directory for a `mix compile` invoked from
+  # elsewhere - and the answer now decides whether the output is deleted.
+  defp source do
+    if src = Mix.Project.config()[:appup] do
+      Path.expand(src, Path.dirname(Mix.Project.project_file()))
+    end
+  end
+
+  defp destination do
+    Path.join(Mix.Project.compile_path(), "#{Mix.Project.config()[:app]}.appup")
+  end
+
+  # Diagnostics returned from a compiler are for editors to display inline;
+  # nothing on the command line prints them, and a compiler that fails without
+  # saying why is worse than one that does not fail at all. Say it on stderr as
+  # well, the way the rest of Forecastle reports build problems.
+  defp report(severity, message) do
+    Mix.shell().error("#{severity}: #{message}")
+    [diagnostic(severity, message)]
+  end
+
+  defp diagnostic(severity, message) do
     %Mix.Task.Compiler.Diagnostic{
       compiler_name: "Appup",
-      file: file,
+      file: Mix.Project.project_file(),
       position: nil,
       severity: severity,
       message: message

test/fixtures/sample/mix.exs

@@ -8,7 +8,7 @@ defmodule Sample.MixProject do
       app: :sample,
       version: System.get_env("SAMPLE_VSN", "0.1.0"),
       elixir: "~> 1.18",
-      appup: "appup.exs",
+      appup: appup(),
       compilers: Mix.compilers() ++ [:appup],
       deps: deps(),
       releases: releases()
@@ -40,6 +40,19 @@ defmodule Sample.MixProject do
     ]
   end
 
+  # Switched by the test suite so that the fixture can be built as a project
+  # that does not ask for an appup at all. "none" rather than an empty value,
+  # which `System.cmd/3` cannot pass: it unsets the variable instead. `nil` is
+  # as close as this can get to dropping the key, and it is close enough - the
+  # compiler reads it through `Mix.Project.config()[:appup]`, which cannot tell
+  # the two apart.
+  defp appup do
+    case System.get_env("SAMPLE_APPUP", "appup.exs") do
+      "none" -> nil
+      path -> path
+    end
+  end
+
   # Switched by the test suite so that the warning about unsupported Windows
   # executables can be provoked.
   defp executables do