From baaca733786a1c748df289a980d6bade6b29e51d Mon Sep 17 00:00:00 2001
From: Alejandro R Mosteo <amosteo@unizar.es>
Date: Mon, 4 Sep 2023 13:33:01 +0200
Subject: [PATCH] Add crate config variables to build hash (#1434)

* Add crate configuration values to build hash inputs

* Tests for configuration hashing
---
 src/alire/alire-builds-hashes.adb             | 27 ++++++++----
 .../alire-crate_configuration-hashes.adb      | 42 +++++++++++++++++++
 .../alire-crate_configuration-hashes.ads      | 10 +++++
 src/alire/alire-crate_configuration.adb       |  9 +++-
 src/alire/alire-crate_configuration.ads       |  3 ++
 src/alire/alire-roots.adb                     | 21 +++++++++-
 testsuite/drivers/builds.py                   | 22 +++++++++-
 .../li/libhello/libhello-0.9.0.toml           | 12 ++++++
 .../cr/crate_real/crate_real-1.0.0.toml       |  5 ++-
 .../tests/build/hashes/compiler-input/test.py | 13 +++---
 .../build/hashes/compiler-missing/test.py     |  5 ++-
 .../tests/build/hashes/config-types/test.py   | 30 +++++++++++++
 .../tests/build/hashes/config-types/test.yaml |  3 ++
 .../tests/build/hashes/hashing-inputs/test.py |  1 +
 .../build/hashes/incomplete-config/test.py    | 28 +++++++++++++
 .../build/hashes/incomplete-config/test.yaml  |  3 ++
 testsuite/tests/config/shared-deps/test.py    | 16 ++++---
 .../dockerized/misc/default-cache/test.py     |  6 ++-
 18 files changed, 224 insertions(+), 32 deletions(-)
 create mode 100644 src/alire/alire-crate_configuration-hashes.adb
 create mode 100644 src/alire/alire-crate_configuration-hashes.ads
 create mode 100644 testsuite/fixtures/build_hash_index/li/libhello/libhello-0.9.0.toml
 create mode 100644 testsuite/tests/build/hashes/config-types/test.py
 create mode 100644 testsuite/tests/build/hashes/config-types/test.yaml
 create mode 100644 testsuite/tests/build/hashes/incomplete-config/test.py
 create mode 100644 testsuite/tests/build/hashes/incomplete-config/test.yaml

diff --git a/src/alire/alire-builds-hashes.adb b/src/alire/alire-builds-hashes.adb
index 27f532f9..f272b260 100644
--- a/src/alire/alire-builds-hashes.adb
+++ b/src/alire/alire-builds-hashes.adb
@@ -1,3 +1,4 @@
+with Alire.Crate_Configuration.Hashes;
 with Alire.Directories;
 with Alire.Environment;
 with Alire.GPR;
@@ -172,17 +173,27 @@ package body Alire.Builds.Hashes is
             end loop;
          end Add_Environment;
 
+         -----------------------
+         -- Add_Configuration --
+         -----------------------
+
+         procedure Add_Configuration is
+         begin
+            Crate_Configuration.Hashes.Add_From
+              (Config => Root.Configuration,
+               Rel    => Rel,
+               Add    => Add'Access);
+         end Add_Configuration;
+
       begin
          Trace.Debug ("   build hashing: " & Rel.Milestone.TTY_Image);
 
          --  Add individual contributors to the hash input
-         Add_Profile;
-         Add_Externals;
-         Add_Environment;
-         Add_Compiler;
-
-         --  Configuration variables
-         --  TBD
+         Add_Profile;       -- Build profile
+         Add_Externals;     -- GPR externals
+         Add_Environment;   -- Environment variables
+         Add_Compiler;      -- Compiler version
+         Add_Configuration; -- Crate configuration variables
 
          --  Dependencies recursive hash? Since a crate can use a dependency
          --  config spec, it is possible in the worst case for a crate to
@@ -213,6 +224,8 @@ package body Alire.Builds.Hashes is
       Environment.Load (Context, Root, For_Hashing => True);
       Env := Context.Get_All;
 
+      Root.Configuration.Ensure_Complete;
+
       for Rel of Root.Solution.Releases loop
          if Root.Requires_Build_Sync (Rel) then
             Compute (Rel);
diff --git a/src/alire/alire-crate_configuration-hashes.adb b/src/alire/alire-crate_configuration-hashes.adb
new file mode 100644
index 00000000..a6f13936
--- /dev/null
+++ b/src/alire/alire-crate_configuration-hashes.adb
@@ -0,0 +1,42 @@
+with Alire.Errors;
+
+package body Alire.Crate_Configuration.Hashes is
+
+   procedure Add_From (Config : Global_Config;
+                       Rel    : Releases.Release;
+                       Add    : access procedure (Kind, Key, Value : String))
+   is
+   begin
+      --  We could iterate over Config contents, but by relying on the release
+      --  reported variables we ensure nothing is amiss during hashing.
+
+      for Untyped of Rel.Config_Variables loop
+         declare
+            Def : constant Properties.Configurations.Config_Type_Definition :=
+                    Properties.Configurations.Config_Type_Definition (Untyped);
+            Key : constant String :=
+                    Crate_Configuration.Key (Rel.Name, Def.Name);
+         begin
+            if not Config.Var_Map.Contains (+Key) then
+               raise Program_Error
+                 with Errors.Set
+                   ("Incomplete configuration during hashing, missing value "
+                    & "for: " & Key);
+            end if;
+
+            if not Def.Valid (Config.Var_Map (+Key).Value) then
+               raise Program_Error
+                 with Errors.Set ("Invalid config value during hashing: "
+                                  & Def.Image);
+            end if;
+
+            Add (Kind  => "config",
+                 Key   => Key,
+                 Value =>
+                   Properties.Configurations.Image
+                     (Config.Var_Map (+Key).Value));
+         end;
+      end loop;
+   end Add_From;
+
+end Alire.Crate_Configuration.Hashes;
diff --git a/src/alire/alire-crate_configuration-hashes.ads b/src/alire/alire-crate_configuration-hashes.ads
new file mode 100644
index 00000000..a3388101
--- /dev/null
+++ b/src/alire/alire-crate_configuration-hashes.ads
@@ -0,0 +1,10 @@
+with Alire.Releases;
+
+package Alire.Crate_Configuration.Hashes is
+
+   procedure Add_From (Config : Global_Config;
+                       Rel    : Releases.Release;
+                       Add    : access procedure (Kind, Key, Value : String));
+   --  Add configuration values of the given crate to the build hash input
+
+end Alire.Crate_Configuration.Hashes;
diff --git a/src/alire/alire-crate_configuration.adb b/src/alire/alire-crate_configuration.adb
index b130e206..f7c5edf9 100644
--- a/src/alire/alire-crate_configuration.adb
+++ b/src/alire/alire-crate_configuration.adb
@@ -734,6 +734,13 @@ package body Alire.Crate_Configuration is
       TIO.Close (File);
    end Generate_C_Config;
 
+   ---------
+   -- Key --
+   ---------
+
+   function Key (Crate : Crate_Name; Var_Name : String) return String
+   is (To_Lower_Case (Crate.As_String & "." & Var_Name));
+
    --------------------
    -- Add_Definition --
    --------------------
@@ -744,7 +751,7 @@ package body Alire.Crate_Configuration is
    is
       Type_Name_Lower : constant String := To_Lower_Case (Type_Def.Name);
 
-      Name : constant Unbounded_String := +(+Crate & "." & Type_Name_Lower);
+      Name : constant Unbounded_String := +Key (Crate, Type_Def.Name);
    begin
 
       if Is_Reserved_Name (Type_Name_Lower) then
diff --git a/src/alire/alire-crate_configuration.ads b/src/alire/alire-crate_configuration.ads
index cc08f6fe..292e4d76 100644
--- a/src/alire/alire-crate_configuration.ads
+++ b/src/alire/alire-crate_configuration.ads
@@ -172,4 +172,7 @@ private
                                 Filepath : Absolute_Path;
                                 Version  : String);
 
+   function Key (Crate : Crate_Name; Var_Name : String) return String;
+   --  Keys in the var map are normalized as "crate.var" lowercased
+
 end Alire.Crate_Configuration;
diff --git a/src/alire/alire-roots.adb b/src/alire/alire-roots.adb
index 420b653d..03c8bc99 100644
--- a/src/alire/alire-roots.adb
+++ b/src/alire/alire-roots.adb
@@ -212,7 +212,7 @@ package body Alire.Roots is
          --  Changes in configuration may require new build dirs
       end if;
 
-      if Export_Build_Env then
+      if Export_Build_Env or else not Builds.Sandboxed_Dependencies then
          This.Export_Build_Environment;
       end if;
 
@@ -765,7 +765,24 @@ package body Alire.Roots is
       --  dependencies from there. Post-fetch may happen even with shared
       --  builds for linked and binary dependencies.
 
-      This.Export_Build_Environment;
+      if Builds.Sandboxed_Dependencies then
+         This.Export_Build_Environment;
+      else
+         null;
+         --  When using shared dependencies we have a conflict between crates
+         --  "in-place" (without syncing, e.g. links/binaries), which should
+         --  have its post-fetch run immediately, and regular crates, which
+         --  get the post-fetch run after sync. Since the complete environment
+         --  cannot be known for the former until build time (as config could
+         --  be incomplete otherwise), we need to delay post-fetch for all
+         --  crates to build time, in a follow-up PR. Meanwhile, in some corner
+         --  cases post-fetch could fail when using shared deps (in-place
+         --  crates with a post-fetch that relies on the environment).
+
+         --  TODO: delay post-fetch for binary/linked crates to the build
+         --  moment too. Do this for both sandboxed/shared, for the sake
+         --  of simplicity?
+      end if;
 
       --  Visit dependencies in a safe order to be fetched, and their actions
       --  ran
diff --git a/testsuite/drivers/builds.py b/testsuite/drivers/builds.py
index 7a2f73d5..32397ced 100644
--- a/testsuite/drivers/builds.py
+++ b/testsuite/drivers/builds.py
@@ -5,7 +5,8 @@ Helper functions for the testing of shared builds
 from glob import glob
 import os
 from shutil import rmtree
-from drivers.alr import alr_builds_dir
+import subprocess
+from drivers.alr import alr_builds_dir, run_alr
 
 
 def clear_builds_dir() -> None:
@@ -45,4 +46,21 @@ def path() -> str:
     """
     Return the path to the shared build directory.
     """
-    return alr_builds_dir()
\ No newline at end of file
+    return alr_builds_dir()
+
+
+def sync() -> None:
+    """
+    Sync the shared build directory
+    """
+    # We force the sync by running a build, no matter if it succeeds or not
+    try:
+        subprocess.run(["alr", "-q", "-d", "build"]
+                       , stdout=subprocess.DEVNULL
+                       , stderr=subprocess.DEVNULL
+                       )
+    except:
+        pass
+
+def sync_builds() -> None:
+    sync()
\ No newline at end of file
diff --git a/testsuite/fixtures/build_hash_index/li/libhello/libhello-0.9.0.toml b/testsuite/fixtures/build_hash_index/li/libhello/libhello-0.9.0.toml
new file mode 100644
index 00000000..0a2b0cef
--- /dev/null
+++ b/testsuite/fixtures/build_hash_index/li/libhello/libhello-0.9.0.toml
@@ -0,0 +1,12 @@
+description = "\"Hello, world!\" demonstration project support library"
+name = "libhello"
+version = "0.9.0"
+maintainers = ["alejandro@mosteo.com"]
+maintainers-logins = ["mylogin"]
+tags = ["libhello-tag1"]
+
+[configuration.variables]
+Var1={type="Boolean"}  # Without default on purpose
+
+[origin]
+url = "file:../../../crates/libhello_1.0.0"
diff --git a/testsuite/fixtures/toolchain_index/cr/crate_real/crate_real-1.0.0.toml b/testsuite/fixtures/toolchain_index/cr/crate_real/crate_real-1.0.0.toml
index 27fa5055..23025fad 100644
--- a/testsuite/fixtures/toolchain_index/cr/crate_real/crate_real-1.0.0.toml
+++ b/testsuite/fixtures/toolchain_index/cr/crate_real/crate_real-1.0.0.toml
@@ -4,7 +4,7 @@ version = "1.0.0"
 maintainers = ["alejandro@mosteo.com"]
 maintainers-logins = ["mylogin"]
 
-# Externals and environment used to track build hash generation immutability
+# Externals, environment, config used to track build hash generation immutability
 
 [gpr-externals]
 foo = ""
@@ -15,6 +15,9 @@ bar = "foo"
 [environment]
 BAZ.set = "foo"
 
+[configuration.variables]
+Var1={type="Boolean", default=true}
+
 [origin]
 url = "file:../../../crates/libhello_1.0.0.tgz"
 hashes = ["sha512:99fa3a55540d0655c87605b54af732f76a8a363015f183b06e98aa91e54c0e69397872718c5c16f436dd6de0fba506dc50c66d34a0e5c61fb63cb01fa22f35ac"]
diff --git a/testsuite/tests/build/hashes/compiler-input/test.py b/testsuite/tests/build/hashes/compiler-input/test.py
index 5a127ac3..16090b69 100644
--- a/testsuite/tests/build/hashes/compiler-input/test.py
+++ b/testsuite/tests/build/hashes/compiler-input/test.py
@@ -7,6 +7,7 @@ import sys
 from drivers.alr import external_compiler_version, run_alr, init_local_crate, alr_with
 from drivers.asserts import assert_match
 from drivers.builds import clear_builds_dir, hash_input
+from drivers import builds
 
 
 def check_hash(signature: str) -> None:
@@ -28,7 +29,7 @@ run_alr("config", "--set", "--global", "dependencies.shared", "true")
 # Init a crate without explicit compiler dependency
 init_local_crate("xxx")
 alr_with("crate_real")  # A regular crate in the index
-run_alr("update")       # Ensure the hash inputs are written to disk
+builds.sync()           # Ensure the hash inputs are written to disk
 
 # Check the external compiler is in the hash inputs
 check_hash(f"version:gnat_external={external_compiler_version()}")
@@ -40,11 +41,7 @@ check_hash(f"version:gnat_external={external_compiler_version()}")
 run_alr("toolchain", "--select", "gnat_native")
 # Clear the build cache so we are able to locate the new hash
 clear_builds_dir()
-run_alr("update")
-run_alr("update")
-# Twice necessary because otherwise the hash inputs cannot be written (during
-# the first update the destination folder does not yet exist, and the crate
-# sync would remove the hash inputs file)
+builds.sync()
 
 # Check the expected compiler is in the hash inputs
 check_hash("version:gnat_native=8888.0.0")
@@ -55,7 +52,7 @@ check_hash("version:gnat_native=8888.0.0")
 
 clear_builds_dir()
 alr_with("gnat=7777")  # Downgrade the compiler with an explicit dependency
-run_alr("update")
+builds.sync()
 
 # Check the expected compiler is in the hash inputs
 check_hash("version:gnat_native=7777.0.0")
@@ -67,7 +64,7 @@ check_hash("version:gnat_native=7777.0.0")
 
 clear_builds_dir()
 alr_with("gnat_native")
-run_alr("update")
+builds.sync()
 check_hash("version:gnat_native=7777.0.0")
 
 
diff --git a/testsuite/tests/build/hashes/compiler-missing/test.py b/testsuite/tests/build/hashes/compiler-missing/test.py
index caa3f7f5..8a838a35 100644
--- a/testsuite/tests/build/hashes/compiler-missing/test.py
+++ b/testsuite/tests/build/hashes/compiler-missing/test.py
@@ -13,7 +13,10 @@ run_alr("config", "--set", "--global", "dependencies.shared", "true")
 
 # Init a crate without explicit compiler dependency
 init_local_crate("xxx")
-p = run_alr("with", "libhello", complain_on_error=False)  # This should fail
+run_alr("with", "libhello")
+
+# The build fails because we cannot compute the build hash without a compiler
+p = run_alr("build", complain_on_error=False)
 assert_match(".*Unable to determine compiler version", p.out)
 
 print("SUCCESS")
diff --git a/testsuite/tests/build/hashes/config-types/test.py b/testsuite/tests/build/hashes/config-types/test.py
new file mode 100644
index 00000000..69348e12
--- /dev/null
+++ b/testsuite/tests/build/hashes/config-types/test.py
@@ -0,0 +1,30 @@
+"""
+Check the different config types in the hash inputs
+"""
+
+from drivers.alr import alr_with, external_compiler_version, init_local_crate, run_alr
+from drivers.builds import hash_input
+from drivers.asserts import assert_eq
+from drivers import builds
+
+run_alr("config", "--set", "--global", "dependencies.shared", "true")
+
+init_local_crate()
+alr_with("hello=1.0.1")
+builds.sync()
+
+# Chech that the hash inputs contains exactly what we expect it to contain
+
+assert_eq(
+    'config:hello.var1=true\n'
+    'config:hello.var2=str\n'
+    'config:hello.var3=A\n'
+    'config:hello.var4=0\n'
+    'config:hello.var5=0\n'
+    'config:hello.var6=0.00000000000000E+00\n'
+    'config:hello.var7=0.00000000000000E+00\n'
+    'profile:hello=RELEASE\n'
+    f'version:gnat_external={external_compiler_version()}\n',
+    hash_input("hello"))
+
+print("SUCCESS")
diff --git a/testsuite/tests/build/hashes/config-types/test.yaml b/testsuite/tests/build/hashes/config-types/test.yaml
new file mode 100644
index 00000000..8e25447d
--- /dev/null
+++ b/testsuite/tests/build/hashes/config-types/test.yaml
@@ -0,0 +1,3 @@
+driver: python-script
+indexes:
+    build_hash_index: {}
diff --git a/testsuite/tests/build/hashes/hashing-inputs/test.py b/testsuite/tests/build/hashes/hashing-inputs/test.py
index 88979f81..8158cc44 100644
--- a/testsuite/tests/build/hashes/hashing-inputs/test.py
+++ b/testsuite/tests/build/hashes/hashing-inputs/test.py
@@ -35,6 +35,7 @@ assert hash1 != hash2, "Hashes should be different"
 # profile, compiler version.
 
 assert_eq(
+    'config:libhello.var1=true\n'            # crate config var
     'environment:TEST_ENV=myenv\n'           # plain env var set
     'external:TEST_FREEFORM_UNSET=default\n' # declared unset GPR external
     'external:TEST_GPR_EXTERNAL=gpr_ext_B\n' # declared set GPR external
diff --git a/testsuite/tests/build/hashes/incomplete-config/test.py b/testsuite/tests/build/hashes/incomplete-config/test.py
new file mode 100644
index 00000000..c649ea5a
--- /dev/null
+++ b/testsuite/tests/build/hashes/incomplete-config/test.py
@@ -0,0 +1,28 @@
+"""
+Test that a crate with incomplete config (values without defaults) cannot be
+built/hashed
+"""
+
+import shutil
+from drivers.alr import alr_with, external_compiler_version, init_local_crate, run_alr
+from drivers.builds import find_hash, hash_input
+from drivers.asserts import assert_eq, assert_match
+from drivers import builds
+
+run_alr("config", "--set", "--global", "dependencies.shared", "true")
+
+init_local_crate()
+alr_with("libhello=0.9")
+
+run_alr("build", complain_on_error=False)
+# This must fail as there are unset config varibles, but what really we are
+# interested in is in checking that no build folder with an incorrect hash has
+# been generated
+
+try:
+    hash = hash_input("libhello") # Must fail because no build folder exists
+    assert False, "Build folder with incomplete config should not be hashed"
+except AssertionError:
+    pass
+
+print("SUCCESS")
diff --git a/testsuite/tests/build/hashes/incomplete-config/test.yaml b/testsuite/tests/build/hashes/incomplete-config/test.yaml
new file mode 100644
index 00000000..8e25447d
--- /dev/null
+++ b/testsuite/tests/build/hashes/incomplete-config/test.yaml
@@ -0,0 +1,3 @@
+driver: python-script
+indexes:
+    build_hash_index: {}
diff --git a/testsuite/tests/config/shared-deps/test.py b/testsuite/tests/config/shared-deps/test.py
index 7cb67adf..966680b9 100644
--- a/testsuite/tests/config/shared-deps/test.py
+++ b/testsuite/tests/config/shared-deps/test.py
@@ -5,8 +5,9 @@ Check that globally sharing builds works as expected
 import glob
 import os
 
-from drivers.alr import (alr_builds_dir, alr_vault_dir, alr_with, alr_workspace_cache,
-                         init_local_crate, run_alr)
+from drivers import builds
+from drivers.alr import (alr_builds_dir, alr_vault_dir, alr_with,
+                         alr_workspace_cache, init_local_crate, run_alr)
 from drivers.asserts import assert_contents, assert_file_exists
 from drivers.helpers import lines_of
 
@@ -35,17 +36,14 @@ assert_contents(base := os.path.join(vault_dir, "hello_1.0.1_filesystem"),
 
 # Check the contents in the build dir, that should not include generated config
 # because no build has been attempted yet, hence a sync has not been performed.
+# And, since there's no sync yet, neither the build dir exists:
 
-# We need to find the hash first
-base = glob.glob(os.path.join(build_dir, "hello_1.0.1_filesystem_*"))[0]
-
-assert_contents(base,
-                [f'{base}/alire',
-                 f'{base}/alire/build_hash_inputs'
-                 ])
+assert len(glob.glob(os.path.join(build_dir, "hello_1.0.1_filesystem_*"))) == 0, \
+    "Build dir should not exist yet"
 
 # Do a build, and now the sync should have happened and the build dir be filled
 run_alr("build")
+base = builds.find_dir("hello_1.0.1_filesystem")
 
 assert_contents(base,
                 [f'{base}/alire',
diff --git a/testsuite/tests/dockerized/misc/default-cache/test.py b/testsuite/tests/dockerized/misc/default-cache/test.py
index cbb6b9eb..1437dacc 100644
--- a/testsuite/tests/dockerized/misc/default-cache/test.py
+++ b/testsuite/tests/dockerized/misc/default-cache/test.py
@@ -36,10 +36,14 @@ assert \
 
 # Shared builds
 
+# This generates the synced build dir. It fails because there is no toolchain
+# configured, but that is not relevant for this test.
+run_alr("build", complain_on_error=False)
+
 # We hardcode this hash so we detect unwilling changes to our hashing scheme.
 # Every time this hash changes we must know the reason (changes in the hashing
 # procedures)
-hash = "1a29f0454348e767e78ac6912c7409b374b7bf650e81396c8a8750797ae073eb"
+hash = "d56fceceba3a7deaa9df1a9b43afa4fb9c643bf5245b335cc12ba0ae4df15682"
 assert \
     os.path.isdir(f"{base}/builds/crate_real_1.0.0_filesystem_{hash}"), \
     f"Shared build not found at the expected location: f{contents(base)}"
-- 
2.39.5