From 7b9b4cae1c75627a00b32e848d763806af300f6d Mon Sep 17 00:00:00 2001 From: Alejandro R Mosteo Date: Mon, 21 Aug 2023 16:12:45 +0200 Subject: [PATCH] Use build profile in build hash (#1425) * Compute hash using build profile * Fix problem with multiple hashes in one run Rooted in that default build profiles were used first and later the actual profiles caused different folders to be used, in turn causing errors with the conflicting CRATE_ALIRE_PREFIX variables. * Write hash inputs to build dirs * Testsuite additions and fixes * Self-review * Update pins and dependencies --- alire.toml | 2 +- deps/aaa | 2 +- deps/umwi | 2 +- src/alire/alire-builds-hashes.adb | 155 ++++++++++++++++++ src/alire/alire-builds-hashes.ads | 35 ++++ src/alire/alire-builds.adb | 14 +- src/alire/alire-builds.ads | 8 +- src/alire/alire-hashes-common.adb | 19 +++ src/alire/alire-hashes-common.ads | 10 ++ src/alire/alire-roots-editable.adb | 1 + src/alire/alire-roots.adb | 42 ++++- src/alire/alire-roots.ads | 9 + src/alr/alr-commands-build.adb | 14 ++ testsuite/drivers/builds.py | 40 +++++ .../tests/build/hashes/input-profiles/test.py | 33 ++++ .../build/hashes/input-profiles/test.yaml | 3 + .../dockerized/misc/default-cache/test.py | 8 +- .../tests/misc/sync-missing-deps/test.py | 32 ++-- 18 files changed, 399 insertions(+), 30 deletions(-) create mode 100644 src/alire/alire-builds-hashes.adb create mode 100644 src/alire/alire-builds-hashes.ads create mode 100644 testsuite/drivers/builds.py create mode 100644 testsuite/tests/build/hashes/input-profiles/test.py create mode 100644 testsuite/tests/build/hashes/input-profiles/test.yaml diff --git a/alire.toml b/alire.toml index 8c620105f..aa67ac668 100644 --- a/alire.toml +++ b/alire.toml @@ -45,7 +45,7 @@ windows = { ALIRE_OS = "windows" } # Some dependencies require precise versions during the development cycle: [[pins]] -aaa = { url = "https://github.com/mosteo/aaa", commit = "fbfffb1cb269a852201d172119d94f3024b617f2" } +aaa = { url = "https://github.com/mosteo/aaa", commit = "c3b5a19adac66f42be45e22694c9463997b4f756" } ada_toml = { url = "https://github.com/mosteo/ada-toml", commit = "da4e59c382ceb0de6733d571ecbab7ea4919b33d" } clic = { url = "https://github.com/alire-project/clic", commit = "6879b90876a1c918b4e112f59c6db0e25b713f52" } gnatcoll = { url = "https://github.com/alire-project/gnatcoll-core.git", commit = "4e663b87a028252e7e074f054f8f453661397166" } diff --git a/deps/aaa b/deps/aaa index f60254934..c3b5a19ad 160000 --- a/deps/aaa +++ b/deps/aaa @@ -1 +1 @@ -Subproject commit f60254934a7d6e39b72380b496527295602f75e3 +Subproject commit c3b5a19adac66f42be45e22694c9463997b4f756 diff --git a/deps/umwi b/deps/umwi index c8aabdc73..32496c15f 160000 --- a/deps/umwi +++ b/deps/umwi @@ -1 +1 @@ -Subproject commit c8aabdc73a6cd2d46d80175944dd7d47e090d1b7 +Subproject commit 32496c15fe4fbb6cdab54ea11fbb0815549d2d48 diff --git a/src/alire/alire-builds-hashes.adb b/src/alire/alire-builds-hashes.adb new file mode 100644 index 000000000..e123a2e77 --- /dev/null +++ b/src/alire/alire-builds-hashes.adb @@ -0,0 +1,155 @@ +with Alire.Directories; +with Alire.Hashes.SHA256_Impl; +with Alire.Paths; +with Alire.Roots; +with Alire.Utils.Text_Files; + +package body Alire.Builds.Hashes is + + use Directories.Operators; + + package SHA renames Alire.Hashes.SHA256_Impl; + + subtype Variables is AAA.Strings.Set; + -- We'll store all variables that affect a Release in a deterministic order + + ----------- + -- Clear -- + ----------- + + procedure Clear (This : in out Hasher) is + begin + This.Hashes.Clear; + end Clear; + + -------------- + -- Is_Empty -- + -------------- + + function Is_Empty (This : Hasher) return Boolean + is (This.Hashes.Is_Empty); + + ------------- + -- Compute -- + ------------- + + procedure Compute (This : in out Hasher; + Root : in out Roots.Root) + is + + ------------- + -- Compute -- + ------------- + + procedure Compute (Rel : Releases.Release) is + Vars : Variables; + + --------- + -- Add -- + --------- + + procedure Add (Kind, Key, Value : String) is + use AAA.Strings; + Datum : constant String := + Trim (Kind) & ":" + & Trim (Key) & "=" + & Trim (Value); + begin + Trace.Debug (" build hashing " & Datum); + Vars.Insert (Datum); + end Add; + + ------------------ + -- Compute_Hash -- + ------------------ + + procedure Compute_Hash is + C : SHA.Hashing_Context; + begin + for Var of Vars loop + SHA.Update (C, Var, Append_Nul => True); + -- The nul character as separator ensures no ambiguity because + -- of consecutive entries. + end loop; + + This.Hashes.Insert (Rel.Name, SHA.Get_Digest (C)); + end Compute_Hash; + + ------------------ + -- Write_Inputs -- + ------------------ + + procedure Write_Inputs is + File : constant Absolute_Path := + Builds.Path + / Rel.Base_Folder & "_" & This.Hashes (Rel.Name) + / Paths.Working_Folder_Inside_Root + / "build_hash_inputs"; + use Directories; + use Utils.Text_Files; + + Lines : AAA.Strings.Vector; + begin + -- First ensure we have a pristine file to work with + Delete_Tree (File); + Create_Tree (Parent (File)); + Touch (File); + + -- Now add the hashed contents for the record + + for Var of Vars loop + Lines.Append (Var); + end loop; + + Append_Lines (File, + Lines, + Backup => False); + end Write_Inputs; + + begin + Trace.Debug (" build hashing: " & Rel.Milestone.TTY_Image); + + -- Build profile + Add ("profile", + Rel.Name.As_String, + Root.Configuration.Build_Profile (Rel.Name)'Image); + + -- GPR externals + -- TBD + + -- Environment variables + -- TBD + + -- Configuration variables + -- TBD + + -- Final computation + Compute_Hash; + + -- Write the hash input for the record + Write_Inputs; + + Trace.Debug (" build hashing release complete"); + end Compute; + + begin + Trace.Debug ("build hashing root " & Root.Path); + This.Hashes.Clear; + + for Rel of Root.Solution.Releases loop + if Root.Requires_Build_Sync (Rel) then + Compute (Rel); + end if; + end loop; + end Compute; + + ---------- + -- Hash -- + ---------- + + function Hash (This : in out Hasher; + Name : Crate_Name) + return String + is (This.Hashes (Name)); + +end Alire.Builds.Hashes; diff --git a/src/alire/alire-builds-hashes.ads b/src/alire/alire-builds-hashes.ads new file mode 100644 index 000000000..272c1b7c6 --- /dev/null +++ b/src/alire/alire-builds-hashes.ads @@ -0,0 +1,35 @@ +private with Ada.Containers.Indefinite_Ordered_Maps; + +limited with Alire.Roots; + +package Alire.Builds.Hashes is + + type Hasher is tagged private; + -- Used to compute all build hashes for releases in a build + + procedure Clear (This : in out Hasher); + -- Remove any cached hashes + + function Is_Empty (This : Hasher) return Boolean; + -- Says if the Hasher has been used or not + + procedure Compute (This : in out Hasher; + Root : in out Roots.Root); + -- Compute all hashes needed for a release + + function Hash (This : in out Hasher; + Name : Crate_Name) + return String + with Pre => not This.Is_Empty; + -- Retrieve the hash of a crate in Root's solution + +private + + package Crate_Hash_Maps is new Ada.Containers.Indefinite_Ordered_Maps + (Crate_Name, String); + + type Hasher is tagged record + Hashes : Crate_Hash_Maps.Map; + end record; + +end Alire.Builds.Hashes; diff --git a/src/alire/alire-builds.adb b/src/alire/alire-builds.adb index 8dad9bc42..4ed939c95 100644 --- a/src/alire/alire-builds.adb +++ b/src/alire/alire-builds.adb @@ -6,6 +6,7 @@ with Alire.OS_Lib.Subprocess; with Alire.Paths.Vault; with Alire.Platforms.Current; with Alire.Properties.Actions.Executor; +with Alire.Roots; with Alire.Utils.Tools; package body Alire.Builds is @@ -49,12 +50,13 @@ package body Alire.Builds is -- Sync -- ---------- - procedure Sync (Release : Releases.Release; + procedure Sync (Root : in out Roots.Root; + Release : Releases.Release; Was_There : out Boolean) is Src : constant Absolute_Path := Paths.Vault.Path / Release.Deployment_Folder; - Dst : constant Absolute_Path := Builds.Path (Release); + Dst : constant Absolute_Path := Builds.Path (Root, Release); Completed : Directories.Completion := Directories.New_Completion (Dst); use AAA.Strings; @@ -126,10 +128,12 @@ package body Alire.Builds is -- Path -- ---------- - function Path (Release : Releases.Release) return Absolute_Path + function Path (Root : in out Roots.Root; + Release : Releases.Release) + return Absolute_Path is (Builds.Path / (Release.Deployment_Folder - & "_deadbeef")); - -- TODO: implement actual hashing of environment for a release + & "_" + & Root.Build_Hash (Release.Name))); end Alire.Builds; diff --git a/src/alire/alire-builds.ads b/src/alire/alire-builds.ads index 4069a38e4..0735f6167 100644 --- a/src/alire/alire-builds.ads +++ b/src/alire/alire-builds.ads @@ -1,4 +1,5 @@ with Alire.Releases; +limited with Alire.Roots; package Alire.Builds is @@ -29,14 +30,17 @@ package Alire.Builds is function Sandboxed_Dependencies return Boolean; -- Queries config to see if dependencies should be sandboxed in workspace - procedure Sync (Release : Releases.Release; + procedure Sync (Root : in out Roots.Root; + Release : Releases.Release; Was_There : out Boolean) with Pre => Release.Origin.Requires_Build; function Path return Absolute_Path; -- Location of shared builds - function Path (Release : Releases.Release) return Absolute_Path; + function Path (Root : in out Roots.Root; + Release : Releases.Release) + return Absolute_Path; -- Computes the complete path in which the release is going to be built end Alire.Builds; diff --git a/src/alire/alire-hashes-common.adb b/src/alire/alire-hashes-common.adb index d650e8813..e040c8bdf 100644 --- a/src/alire/alire-hashes-common.adb +++ b/src/alire/alire-hashes-common.adb @@ -32,6 +32,25 @@ package body Alire.Hashes.Common is raise; end Hash_File; + ------------ + -- Update -- + ------------ + + procedure Update (C : in out Context; + S : String; + Append_Nul : Boolean := True) + is + use Ada.Streams; + Bytes : Stream_Element_Array (1 .. S'Length) + with Address => S (S'First)'Address, Import; + pragma Assert (Bytes'Size = S (S'Range)'Size); + begin + Update (C, Bytes); + if Append_Nul then + Update (C, Stream_Element_Array'(1 .. 1 => 0)); + end if; + end Update; + begin Hashes.Hash_Functions (Kind) := Hash_File'Access; end Alire.Hashes.Common; diff --git a/src/alire/alire-hashes-common.ads b/src/alire/alire-hashes-common.ads index 958b737ae..8321c3a19 100644 --- a/src/alire/alire-hashes-common.ads +++ b/src/alire/alire-hashes-common.ads @@ -12,6 +12,10 @@ generic with function Digest (C : Context) return String is <>; package Alire.Hashes.Common is + subtype Hashing_Context is Context; + function Get_Digest (C : Context) return String renames Digest; + -- Reexpose formals to gain visibility outside the generic + function Hash_File (Path : File_Path) return Any_Hash; -- This function does not need to be visible (it is not used directly), but -- hiding it in the body results in the following error in FSF compilers: @@ -24,4 +28,10 @@ package Alire.Hashes.Common is -- gprbind: invocation of gnatbind failed -- gprbuild: unable to bind alr-main.adb + procedure Update (C : in out Context; + S : String; + Append_Nul : Boolean := True); + -- Convenience to directly hash lists of strings. To avoid ambiguities, by + -- default a NUL char is used to separate such strings. + end Alire.Hashes.Common; diff --git a/src/alire/alire-roots-editable.adb b/src/alire/alire-roots-editable.adb index d26294eca..55c9565a1 100644 --- a/src/alire/alire-roots-editable.adb +++ b/src/alire/alire-roots-editable.adb @@ -64,6 +64,7 @@ package body Alire.Roots.Editable is Changed_Only => not Alire.Detailed) then Edited.Commit; + Edited.Deploy_Dependencies; else Trace.Info ("No changes applied."); end if; diff --git a/src/alire/alire-roots.adb b/src/alire/alire-roots.adb index 20b06506d..f52b2662c 100644 --- a/src/alire/alire-roots.adb +++ b/src/alire/alire-roots.adb @@ -192,6 +192,7 @@ package body Alire.Roots is if Saved_Profiles then This.Set_Build_Profiles (Crate_Configuration.Last_Build_Profiles); + This.Build_Hasher.Clear; end if; -- Check if crate configuration should be re-generated. This is the old @@ -202,6 +203,9 @@ package body Alire.Roots is and then This.Configuration.Must_Regenerate then This.Generate_Configuration; + elsif not Builds.Sandboxed_Dependencies then + This.Deploy_Dependencies; + -- Changes in configuration may require new build dirs end if; This.Configuration.Ensure_Complete; @@ -231,6 +235,22 @@ package body Alire.Roots is end return; end Build_Context; + ---------------- + -- Build_Hash -- + ---------------- + + function Build_Hash (This : in out Root; + Name : Crate_Name) + return String + is + begin + if This.Build_Hasher.Is_Empty then + This.Build_Hasher.Compute (This); + end if; + + return This.Build_Hasher.Hash (Name); + end Build_Hash; + ------------- -- Install -- ------------- @@ -721,7 +741,7 @@ package body Alire.Roots is -- Sync sources to its shared build location if not Builds.Sandboxed_Dependencies then - Builds.Sync (Rel, Was_There); + Builds.Sync (This, Rel, Was_There); end if; -- At this point, post-fetch have been run by either @@ -1172,6 +1192,9 @@ package body Alire.Roots is is begin This.Cached_Solution.Set (Solution, This.Lock_File); + + -- Invalidate hashes as the new solution may contain new releases + This.Build_Hasher.Clear; end Set; -------------- @@ -1179,7 +1202,17 @@ package body Alire.Roots is -------------- function Solution (This : in out Root) return Solutions.Solution - is (This.Cached_Solution.Element (This.Lock_File)); + is + Result : constant Cached_Solutions.Cached_Info + := This.Cached_Solution.Element (This.Lock_File); + begin + -- Clear hashes in case of manifest change + if not Result.Reused then + This.Build_Hasher.Clear; + end if; + + return Result.Element; + end Solution; ----------------- -- Environment -- @@ -1210,6 +1243,7 @@ package body Alire.Roots is Release => Releases.Containers.To_Release_H (R), Cached_Solution => <>, Configuration => <>, + Build_Hasher => <>, Pins => <>, Lockfile => <>, Manifest => <>); @@ -1323,10 +1357,10 @@ package body Alire.Roots is declare Rel : constant Releases.Release := Release (This, Crate); begin - if Builds.Sandboxed_Dependencies then + if not This.Requires_Build_Sync (Rel) then return This.Release_Parent (Rel, For_Build) / Rel.Base_Folder; else - return Builds.Path (Rel); + return Builds.Path (This, Rel); end if; end; elsif This.Solution.State (Crate).Is_Linked then diff --git a/src/alire/alire-roots.ads b/src/alire/alire-roots.ads index e4424d02e..4136b2f4d 100644 --- a/src/alire/alire-roots.ads +++ b/src/alire/alire-roots.ads @@ -3,6 +3,7 @@ private with Ada.Finalization; with AAA.Strings; +private with Alire.Builds.Hashes; with Alire.Containers; with Alire.Crate_Configuration; with Alire.Dependencies.States; @@ -256,6 +257,11 @@ package Alire.Roots is -- the ones given in This.Configuration are used. These come in order of -- increasing priority from: defaults -> manifests -> explicit set via API. + function Build_Hash (This : in out Root; + Name : Crate_Name) + return String; + -- Returns the build hash of a crate if the solution; computes on demand. + procedure Install (This : in out Root; Prefix : Absolute_Path; @@ -353,6 +359,9 @@ private -- versions. As a data point, with the stock Ubuntu 20.04 GNAT (9.3), -- there is no problem. + Build_Hasher : Builds.Hashes.Hasher; + -- Used to compute the build hashes of releases in the solution + Pins : Solutions.Solution; -- Closure of all pins that are recursively found diff --git a/src/alr/alr-commands-build.adb b/src/alr/alr-commands-build.adb index 25ef645c9..8616a53be 100644 --- a/src/alr/alr-commands-build.adb +++ b/src/alr/alr-commands-build.adb @@ -1,3 +1,4 @@ +with Alire.Builds; with Alire.Crate_Configuration; with Alire.Utils.Switches; @@ -61,6 +62,14 @@ package body Alr.Commands.Build is Reportaise_Wrong_Arguments ("Only one build profile can be selected"); end if; + -- Prevent premature update of dependencies, as the exact folders + -- will depend on the build hashes, which are yet unknown until + -- build profiles are applied. + Cmd.Requires_Workspace (Sync => Alire.Builds.Sandboxed_Dependencies); + -- For sandboxed dependencies we keep the legacy behavior. We can unify + -- behaviors when crate configuration is only generated per missing + -- crate. + -- Build profile in the command line takes precedence. The configuration -- will have been loaded at this time with all profiles found in -- manifests. @@ -102,6 +111,11 @@ package body Alr.Commands.Build is return Boolean is begin + -- Prevent premature update of dependencies, as the exact folders + -- will depend on the build hashes, which are yet unknown until + -- build profiles are applied. + Cmd.Requires_Workspace (Sync => Alire.Builds.Sandboxed_Dependencies); + -- TODO: remove sync once config generation is per crate. declare Timer : Stopwatch.Instance; diff --git a/testsuite/drivers/builds.py b/testsuite/drivers/builds.py new file mode 100644 index 000000000..ed1bee964 --- /dev/null +++ b/testsuite/drivers/builds.py @@ -0,0 +1,40 @@ +""" +Helper functions for the testing of shared builds +""" + +from glob import glob +import os +from drivers.alr import alr_builds_dir + + +def find_dir(crate_name: str) -> str: + """ + Find the build dir of a crate in the shared build directory + """ + if len(found := glob(f"{path()}/{crate_name}_*")) != 1: + raise AssertionError(f"Unexpected number of dirs for crate {crate_name}: {found}") + return glob(f"{path()}/{crate_name}_*")[0] + + +def find_hash(crate_name: str) -> str: + """ + Find the hash of a crate in the shared build directory + """ + return find_dir(crate_name).split("_")[-1] + + +def hash_input(crate_name: str, as_lines: bool=False) -> str: + """ + Return the hash inputs for a crate build dir + """ + with open(os.path.join(f"{find_dir(crate_name)}", + "alire", + "build_hash_inputs")) as f: + return f.readlines() if as_lines else f.read() + + +def path() -> str: + """ + Return the path to the shared build directory. + """ + return alr_builds_dir() \ No newline at end of file diff --git a/testsuite/tests/build/hashes/input-profiles/test.py b/testsuite/tests/build/hashes/input-profiles/test.py new file mode 100644 index 000000000..4af2a0e3f --- /dev/null +++ b/testsuite/tests/build/hashes/input-profiles/test.py @@ -0,0 +1,33 @@ +""" +Test that the inputs to the hashing properly reflect the build profile +""" + +import shutil +from drivers.alr import alr_with, init_local_crate, run_alr +from drivers.builds import find_hash, hash_input +from drivers.asserts import assert_match +from drivers import builds + +run_alr("config", "--set", "--global", "dependencies.shared", "true") +init_local_crate() +alr_with("libhello") + +# Build the crate in default mode, so dependencies are in RELEASE mode +run_alr("build") +hash1 = find_hash("libhello") +assert_match(".*profile:libhello=RELEASE.*", + hash_input("libhello")) + +# Build with dependencies in VALIDATION mode +# Clean up first because find_hash() will fail if there are multiple builds +shutil.rmtree(builds.path()) +run_alr("build", "--profiles=*=validation") +hash2 = find_hash("libhello") +assert_match(".*profile:libhello=VALIDATION.*", + hash_input("libhello")) + +# Check that the hashes are different +assert hash1 != hash2, "Hashes should be different" + + +print("SUCCESS") diff --git a/testsuite/tests/build/hashes/input-profiles/test.yaml b/testsuite/tests/build/hashes/input-profiles/test.yaml new file mode 100644 index 000000000..872fc1274 --- /dev/null +++ b/testsuite/tests/build/hashes/input-profiles/test.yaml @@ -0,0 +1,3 @@ +driver: python-script +indexes: + basic_index: {} diff --git a/testsuite/tests/dockerized/misc/default-cache/test.py b/testsuite/tests/dockerized/misc/default-cache/test.py index 068063efc..1a2367a4c 100644 --- a/testsuite/tests/dockerized/misc/default-cache/test.py +++ b/testsuite/tests/dockerized/misc/default-cache/test.py @@ -4,7 +4,6 @@ """ import os -import sys from drivers.alr import alr_with, init_local_crate, run_alr from drivers.helpers import contents @@ -36,9 +35,10 @@ f"Vault not found at the expected location: f{contents(base)}" # Shared builds +# We hardcode this hash so we detect unwilling changes to our hashing scheme +hash = "e66592c9a181de97dc3a342cf76378f6ffa6667d7c1864c74d98bec8ffaf4f3d" assert \ - os.path.isdir(f"{base}/builds/crate_real_1.0.0_filesystem_deadbeef"), \ - "Vault not found at the expected location: f{contents(base)" - # TODO: above hash will need updating once hash computation is in place + 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)}" print('SUCCESS') diff --git a/testsuite/tests/misc/sync-missing-deps/test.py b/testsuite/tests/misc/sync-missing-deps/test.py index 8bdc0c63c..ba91edda1 100644 --- a/testsuite/tests/misc/sync-missing-deps/test.py +++ b/testsuite/tests/misc/sync-missing-deps/test.py @@ -3,11 +3,10 @@ """ import os.path - -from drivers.alr import run_alr from shutil import rmtree -# from drivers.asserts import assert_eq, assert_match +from drivers.alr import run_alr +from drivers.builds import find_hash # Create a new project and set up dependencies run_alr('init', '--bin', 'xxx') @@ -21,15 +20,24 @@ # Run commands that require a valid session after deleting a dependency. All # should succeed and recreate the missing dependency folder. -for cmd in ['build', 'pin', 'run', 'show', 'with', 'printenv']: - # Delete folder - rmtree(target) - - # Run the command - run_alr(cmd) - - # The successful run should be proof enough, but check folder is there: - assert os.path.isdir(target), "Directory missing at expected location" +# The first round uses sandboxed dependencies. The second round uses shared ones. +for round in range(2): + if round == 2: + # Prepare same test for shared dependencies + run_alr("config", "--set", "--global", "dependencies.shared", "true") + run_alr("update") + target = f"builds.path()/hello_1.0.1_filesystem_{find_hash('hello')}" + + for cmd in ['build', 'pin', 'run', 'show', 'with', 'printenv']: + # Delete folder + rmtree(target) + + # Run the command + run_alr(cmd) + + # The successful run should be proof enough, but check folder is there: + assert os.path.isdir(target), \ + f"Directory missing at expected location after running command: {cmd}" print('SUCCESS')