Merge pull request #20 from ausimian/issue/13-relfile

fix: accept the two release files an unpacked release has

Author: ausimian

AGENTS.md

@@ -388,6 +388,15 @@ puts the other on the peer's code path, and asserts that the answer came from
 the peer's. Peer cleanup is asserted from outside: a provider records the
 operating system pid of the VM it ran in, and the test waits for it to go.
 
+It builds the **unpacked** shape by default, and the assembled one only when a
+test asks for it — because the peer path is reached from `install` and `commit`,
+so every version it is asked to configure was unpacked from a tarball. The two
+shapes differ in the version directory's release files: Mix assembles one,
+`<name>.rel`, while unpacking leaves two, that one plus the `<name>-<vsn>.rel`
+`release_handler` copies in beside it. A fixture that only built the assembled
+shape is how `Castle.Peer` came to refuse every unpacked release as ambiguous,
+having been reviewed seven times against a directory the peer path never meets.
+
 `Castle.Peer.materialise/2` takes `:boot_timeout` and `:resolve_timeout` for the
 same reason `Castle.Commands` takes the module to talk to — a deadline nothing
 can shorten is a deadline no test can show is enforced. Two tests give it a

lib/castle/peer.ex

@@ -467,9 +467,43 @@ defmodule Castle.Peer do
   end
 
   # Mix names the release file after the release, so it is found rather than
-  # named. A version directory holds exactly one: Mix renames the one belonging
-  # to the `start` script and removes the rest. Listed rather than globbed,
-  # since a release root is a path and not a pattern.
+  # named. A version directory holds one or two of them, and which it holds says
+  # how the version got there.
+  #
+  # Mix's copy is `<name>.rel` - the release file belonging to the `start`
+  # script, renamed after the release - and a version directory as Mix
+  # assembled it holds that one alone. Unpacking a tarball into it adds a
+  # second: `release_handler` extracts the tarball's `<name>-<vsn>.rel`, reads
+  # *that* to decide what the version is - `check_rel` on the way in, and the
+  # `RELEASES` entry is written from the record it returns - and then copies it
+  # into the version directory, "keeping this for backwards compatibility
+  # reasons with older systools:make_tar, where there is no copy of the .rel
+  # file in the releases/<vsn> dir. See OTP-9746." For a tarball `systools`
+  # built the two names are the same and that copy overwrites Mix's, which is
+  # why the comment reads as harmless. They differ here, so both survive, and
+  # every unpacked release has two.
+  #
+  # Of the two, `release_handler`'s copy is the authoritative one: it is the
+  # file the version was admitted on, and the applications and emulator version
+  # `RELEASES` records came from those very bytes, so it is the release as the
+  # system understands it. The two are byte-identical in practice - Forecastle
+  # writes the tarball's copy from Mix's with `File.cp!`, the tar carries it
+  # unchanged, and `release_handler`'s copy is a read and a write of the whole
+  # file - and nothing here compares them, deliberately. This is a choice of
+  # which file is authoritative rather than a tie-break between equals, so it
+  # holds however the two differ, and a difference in bytes that cannot change
+  # the answer is no reason to refuse an install that works.
+  #
+  # Anything other than those two shapes is refused, naming what it found: two
+  # names that are not that pair are two release files rather than two copies of
+  # one, and so is any third. The version is all the pair is recognised by, and
+  # it is the version directory's own name - `release_handler` copies into
+  # `releases/<Vsn>` for the `Vsn` it has just read out of the file, and Mix
+  # assembles into `releases/<version>` - so the directory is the version, and
+  # the release name never has to be known here.
+  #
+  # Listed rather than globbed, since a release root is a path and not a
+  # pattern.
   defp release_file(rel_vsn_dir) do
     case File.ls(rel_vsn_dir) do
       {:ok, entries} ->
@@ -489,11 +523,34 @@ defmodule Castle.Peer do
   end
 
   defp release_file(rel_vsn_dir, names) do
-    {:error,
-     "Found more than one release file in #{rel_vsn_dir} - #{Enum.join(names, ", ")} - so " <>
-       "the emulator to evaluate its configuration with is ambiguous."}
+    case unpacked_copy(names, Path.basename(rel_vsn_dir)) do
+      {:ok, name} ->
+        {:ok, Path.join(rel_vsn_dir, name)}
+
+      :none ->
+        {:error,
+         "Found more than one release file in #{rel_vsn_dir} - #{Enum.join(names, ", ")}. An " <>
+           "unpacked version directory holds two, a release file and the copy unpacking " <>
+           "leaves beside it, and these are not that pair - so the emulator to evaluate its " <>
+           "configuration with is ambiguous."}
+    end
+  end
+
+  # The copy `release_handler` made, recognised only while the file it was made
+  # from is still beside it: the one name of the two that is the other with the
+  # version in it.
+  defp unpacked_copy([first, second], vsn) do
+    cond do
+      first == copy_of(second, vsn) -> {:ok, first}
+      second == copy_of(first, vsn) -> {:ok, second}
+      true -> :none
+    end
   end
 
+  defp unpacked_copy(_names, _vsn), do: :none
+
+  defp copy_of(name, vsn), do: "#{Path.rootname(name)}-#{vsn}.rel"
+
   ## sys.config, and the base it is resolved from
 
   # The base is `sys.config.pristine` once there is one, and `sys.config` itself

test/castle/peer_test.exs

@@ -247,12 +247,86 @@ defmodule Castle.PeerTest do
 
     test "reports a version directory with no release file", %{tmp_dir: root} do
       vsn_dir = SyntheticRelease.build(root)
-      File.rm!(Path.join(vsn_dir, "synthetic.rel"))
+      for path <- Path.wildcard(Path.join(vsn_dir, "*.rel")), do: File.rm!(path)
 
       assert {:error, message} = Castle.Peer.materialise(vsn_dir)
       assert message =~ "Cannot find a release file"
     end
 
+    # Every version this path is asked to configure was unpacked from a tarball,
+    # and an unpacked version directory holds two release files: the one Mix put
+    # inside the tarball's version directory, and the copy `release_handler`
+    # makes of the tarball's own `<name>-<vsn>.rel` beside it. That pair is not
+    # ambiguous, and refusing it refused every install - which is what the rest
+    # of this file now builds by default.
+    test "reads the release file in a version directory that was unpacked", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root)
+
+      assert Enum.sort(rel_files(vsn_dir)) == ["synthetic-1.0.0.rel", "synthetic.rel"]
+
+      # The two hold the same bytes, which is why nothing but a difference
+      # planted between them can show which one was read.
+      assert File.read!(Path.join(vsn_dir, "synthetic-1.0.0.rel")) ==
+               File.read!(Path.join(vsn_dir, "synthetic.rel"))
+
+      assert Castle.Peer.materialise(vsn_dir) == {:ok, []}
+    end
+
+    test "reads the copy unpacking left, and not Mix's", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root)
+
+      # Mix's copy, made to name an emulator that is not there. It is the copy
+      # `release_handler` admitted the version on that says what the version is -
+      # `RELEASES` was written from those bytes - so that is the one to read, and
+      # preferring this one would refuse a release that boots.
+      File.write!(
+        Path.join(vsn_dir, "synthetic.rel"),
+        :io_lib.format(~c"~tp.~n", [
+          {:release, {~c"synthetic", ~c"1.0.0"}, {:erts, ~c"0.0.0"}, []}
+        ])
+      )
+
+      assert Castle.Peer.materialise(vsn_dir) == {:ok, []}
+    end
+
+    test "reads the one release file a version Mix assembled has", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root, shape: :assembled)
+
+      assert rel_files(vsn_dir) == ["synthetic.rel"]
+      assert Castle.Peer.materialise(vsn_dir) == {:ok, []}
+    end
+
+    test "refuses two release files that are not one release's", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root, shape: :assembled)
+      File.cp!(Path.join(vsn_dir, "synthetic.rel"), Path.join(vsn_dir, "other.rel"))
+
+      assert {:error, message} = Castle.Peer.materialise(vsn_dir)
+      assert message =~ "Found more than one release file"
+      assert message =~ "synthetic.rel"
+      assert message =~ "other.rel"
+    end
+
+    test "refuses a copy that belongs to another version", %{tmp_dir: root} do
+      # What is accepted is a release file and the copy made of it *for this
+      # version*, since the version directory's own name is the version. A name
+      # that merely looks like such a copy is two release files.
+      vsn_dir = SyntheticRelease.build(root, shape: :assembled)
+      File.cp!(Path.join(vsn_dir, "synthetic.rel"), Path.join(vsn_dir, "synthetic-2.0.0.rel"))
+
+      assert {:error, message} = Castle.Peer.materialise(vsn_dir)
+      assert message =~ "Found more than one release file"
+      assert message =~ "synthetic-2.0.0.rel"
+    end
+
+    test "refuses a third release file beside the pair", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root)
+      File.cp!(Path.join(vsn_dir, "synthetic.rel"), Path.join(vsn_dir, "other.rel"))
+
+      assert {:error, message} = Castle.Peer.materialise(vsn_dir)
+      assert message =~ "Found more than one release file"
+      assert message =~ "other.rel"
+    end
+
     test "gives up on a peer that never boots, at the deadline", %{tmp_dir: root} do
       vsn_dir = SyntheticRelease.build(root, config: with_providers([{PeerProviderStub, []}], []))
       File.write!(Path.join(vsn_dir, "preboot.boot"), "not a boot script")
@@ -783,6 +857,7 @@ defmodule Castle.PeerTest do
                  "preboot.boot",
                  "preboot.script",
                  "synthetic.rel",
+                 "synthetic-1.0.0.rel",
                  "sys.config",
                  "sys.config.pristine",
                  Path.join(work, "sys.config"),
@@ -978,6 +1053,8 @@ defmodule Castle.PeerTest do
 
   defp mode(path), do: Bitwise.band(File.stat!(path).mode, 0o777)
 
+  defp rel_files(vsn_dir), do: Enum.filter(File.ls!(vsn_dir), &String.ends_with?(&1, ".rel"))
+
   # What the peer saw, as paths relative to the version directory.
   defp snapshot(path, relative_to) do
     assert {:ok, [entries]} = :file.consult(to_charlist(path))

test/support/synthetic_release.ex

@@ -15,8 +15,9 @@ defmodule Castle.SyntheticRelease do
   #     applications - kernel, stdlib, sasl, compiler, elixir and castle
   #     started, anything else on the code path only, which is the shape
   #     Forecastle's preboot script has,
-  #   * `releases/<vsn>/<name>.rel`, which is where the emulator version comes
-  #     from, and
+  #   * the release file or files, which is where the emulator version comes
+  #     from - see `write_release_files/4`, since a version directory holds one
+  #     of them or two depending on how the version got there, and
   #   * `releases/<vsn>/sys.config`.
   #
   # The boot script is generated by `systools` from the real application
@@ -25,6 +26,9 @@ defmodule Castle.SyntheticRelease do
 
   @started [:kernel, :stdlib, :sasl, :compiler, :elixir, :castle]
 
+  # The release name, which is what the release file or files are named after.
+  @name "synthetic"
+
   @doc """
   Builds a release under `root` and returns its version directory.
 
@@ -36,6 +40,9 @@ defmodule Castle.SyntheticRelease do
     * `:apps` - `{app, vsn, dir}` triples to add to the code path without
       starting them, which is how a provider module that lives outside the
       applications preboot starts is reached
+    * `:shape` - `:unpacked`, the default, or `:assembled`; which release files
+      the version directory holds, and so how the version is to have got there.
+      See `write_release_files/4`
   """
   def build(root, opts \\ []) do
     vsn = Keyword.get(opts, :vsn, "1.0.0")
@@ -49,7 +56,7 @@ defmodule Castle.SyntheticRelease do
     apps = apps ++ for {app, app_vsn, dir} <- extra, do: {app, app_vsn, :none, dir}
 
     make_boot_script(vsn_dir, vsn, apps, link_apps(lib, apps, extra))
-    write_rel(Path.join(vsn_dir, "synthetic.rel"), vsn, apps)
+    write_release_files(vsn_dir, vsn, apps, Keyword.get(opts, :shape, :unpacked))
     write_sys_config(vsn_dir, opts)
 
     vsn_dir
@@ -159,8 +166,10 @@ defmodule Castle.SyntheticRelease do
   end
 
   # `systools` writes the `.rel` it was given back out beside the boot script,
-  # and a version directory holds exactly one release file, so preboot's goes as
-  # soon as it has been used - which is what Mix does with its own.
+  # so preboot's goes as soon as it has been used - which is what Mix does with
+  # the release file of every boot script but `start`. Left there it would be a
+  # release file no release has, and one `Castle.Peer` would be right to refuse
+  # to choose between.
   defp make_boot_script(vsn_dir, vsn, apps, paths) do
     path = Path.join(vsn_dir, "preboot")
     write_rel(path <> ".rel", vsn, apps)
@@ -178,14 +187,41 @@ defmodule Castle.SyntheticRelease do
     File.rm!(path <> ".rel")
   end
 
+  # The release file, under the name or names the shape being built has.
+  #
+  # `:assembled` is what `mix release` leaves: one file, `<name>.rel`, the one
+  # belonging to the `start` script, which Mix renames after the release.
+  #
+  # `:unpacked` is that plus `<name>-<vsn>.rel`, and is what
+  # `release_handler:unpack_release/1` leaves behind - so it is the shape the
+  # peer path meets, every version it is asked to configure having been unpacked
+  # from a tarball. The second name is the copy Forecastle puts at the top of
+  # the tarball's `releases` directory, which `release_handler` reads to admit
+  # the version and then copies in here, for compatibility with tarballs that
+  # carried no release file inside the version directory. Made by copying,
+  # because copying is how the two come to hold the same bytes in a real
+  # release: `File.cp!` at assembly, and a read and a write of the whole file on
+  # unpack.
+  #
+  # The unpacked shape is the default because it is the one reality presents. A
+  # test that means the assembled shape asks for it.
+  defp write_release_files(vsn_dir, vsn, apps, shape) do
+    assembled = Path.join(vsn_dir, "#{@name}.rel")
+    write_rel(assembled, vsn, apps)
+
+    if shape == :unpacked do
+      File.cp!(assembled, Path.join(vsn_dir, "#{@name}-#{vsn}.rel"))
+    end
+  end
+
   defp write_rel(path, vsn, apps) do
     entries =
       for app_spec <- apps do
         {elem(app_spec, 0), to_charlist(elem(app_spec, 1)), elem(app_spec, 2)}
       end
 
     rel =
-      {:release, {~c"synthetic", to_charlist(vsn)}, {:erts, :erlang.system_info(:version)},
+      {:release, {to_charlist(@name), to_charlist(vsn)}, {:erts, :erlang.system_info(:version)},
        entries}
 
     File.write!(path, :io_lib.format(~c"~tp.~n", [rel]))