From e9678b72f2c999b1ad452f909d7971679c51af70 Mon Sep 17 00:00:00 2001 From: Alejandro R Mosteo Date: Mon, 4 Sep 2023 18:26:15 +0200 Subject: [PATCH] Include hash of dependencies in build hash (#1439) Since a dependency may use different specs based on config values, environment variables, GPR externals... we include the hash of dependencies in dependents' hash inputs. --- src/alire/alire-builds-hashes.adb | 100 ++++++++++++++---- testsuite/drivers/builds.py | 2 +- .../tests/build/hashes/compiler-input/test.py | 2 +- .../tests/build/hashes/config-types/test.py | 7 +- .../tests/build/hashes/hashing-inputs/test.py | 33 +++++- .../tests/crate_config/no-rebuilds/test.py | 2 +- 6 files changed, 114 insertions(+), 32 deletions(-) diff --git a/src/alire/alire-builds-hashes.adb b/src/alire/alire-builds-hashes.adb index c682904f..2b263fe0 100644 --- a/src/alire/alire-builds-hashes.adb +++ b/src/alire/alire-builds-hashes.adb @@ -5,6 +5,8 @@ with Alire.GPR; with Alire.Hashes.SHA256_Impl; with Alire.Paths; with Alire.Roots; +with Alire.Solutions; +with Alire.Dependencies.States; with Alire.Utils.Text_Files; package body Alire.Builds.Hashes is @@ -75,8 +77,19 @@ package body Alire.Builds.Hashes is -- of consecutive entries. end loop; - This.Hashes.Insert (Rel.Name, SHA.Get_Digest (C)); - This.Inputs.Insert (Rel.Name, Vars); + -- We can rarely reach here more than once for provided regular + -- releases, so we cannot simply insert once. Instead of including + -- blindly, we double-check things match. + if This.Hashes.Contains (Rel.Name) + and then This.Hashes (Rel.Name) /= SHA.Get_Digest (C) + then + raise Program_Error with + "Conflicting build hashes for release " + & Rel.Milestone.Image; + else + This.Hashes.Include (Rel.Name, SHA.Get_Digest (C)); + This.Inputs.Include (Rel.Name, Vars); + end if; end Compute_Hash; ----------------- @@ -153,6 +166,30 @@ package body Alire.Builds.Hashes is Add => Add'Access); end Add_Configuration; + ---------------------- + -- Add_Dependencies -- + ---------------------- + + procedure Add_Dependencies is + -- Since specs of dependencies may change due to config variables, + -- or sources selected depending on environment variables/GPR + -- externals, the safest course of action is to also include + -- dependency hashes in our own hash. For dependencies without + -- such things the hash won't change anyway. + begin + for Dep of Rel.Flat_Dependencies loop + for Target of Root.Solution.Releases loop + if Target.Origin.Requires_Build + and then Target.Satisfies (Dep) + then + Add ("dependency", + Target.Milestone.Image, + This.Hashes (Target.Name)); + end if; + end loop; + end loop; + end Add_Dependencies; + begin Trace.Debug (" build hashing: " & Rel.Milestone.TTY_Image); @@ -167,25 +204,34 @@ package body Alire.Builds.Hashes is Add_Externals; -- GPR externals Add_Environment; -- Environment variables Add_Compiler; -- Compiler version + Add_Dependencies; -- Hash of dependencies end if; - -- Dependencies recursive hash? Since a crate can use a dependency - -- config spec, it is possible in the worst case for a crate to - -- require unique builds that include their dependencies hash - -- in their own hash. This is likely a corner case, but we can't - -- currently detect it. Two options are to alway err on the side of - -- caution, always including dependencies hashes, or to add some new - -- info in the manifest saying whose crates config affect the crate. - -- We could also enable this recursive hashing globally or per - -- crate... - -- TBD - -- Final computation Compute_Hash; Trace.Debug (" build hashing release complete"); end Compute; + ------------- + -- Compute -- + ------------- + + procedure Compute (unused_Root : in out Roots.Root; + Unused_Sol : Solutions.Solution; + State : Dependencies.States.State) + is + begin + if State.Has_Release + and then State.Release.Origin.Requires_Build + then + -- We cannot filter out provided dependencies because all + -- dependencies may be fulfilled by a provided release that + -- doesn't appear otherwise (as non-provided). + Compute (State.Release); + end if; + end Compute; + Context : Environment.Context; begin @@ -197,11 +243,7 @@ package body Alire.Builds.Hashes is Root.Configuration.Ensure_Complete; - for Rel of Root.Nonabstract_Releases loop -- includes the root release - if Rel.Origin.Requires_Build then - Compute (Rel); - end if; - end loop; + Root.Traverse (Compute'Access); end Compute; ---------------------- @@ -244,12 +286,24 @@ package body Alire.Builds.Hashes is Backup => False); end Write_Inputs; - begin - for Rel of Root.Nonabstract_Releases loop - if Rel.Origin.Requires_Build then - Write_Inputs (Rel); + ------------------ + -- Write_Inputs -- + ------------------ + + procedure Write_Inputs (Unused_Root : in out Roots.Root; + Unused_Sol : Solutions.Solution; + State : Dependencies.States.State) + is + begin + if State.Has_Release + and then State.Release.Origin.Requires_Build + then + Write_Inputs (State.Release); end if; - end loop; + end Write_Inputs; + + begin + Root.Traverse (Write_Inputs'Access); end Write_Inputs; ---------- diff --git a/testsuite/drivers/builds.py b/testsuite/drivers/builds.py index 32397ced..81c56f43 100644 --- a/testsuite/drivers/builds.py +++ b/testsuite/drivers/builds.py @@ -6,7 +6,7 @@ from glob import glob import os from shutil import rmtree import subprocess -from drivers.alr import alr_builds_dir, run_alr +from drivers.alr import alr_builds_dir def clear_builds_dir() -> None: diff --git a/testsuite/tests/build/hashes/compiler-input/test.py b/testsuite/tests/build/hashes/compiler-input/test.py index 16090b69..377bd9ae 100644 --- a/testsuite/tests/build/hashes/compiler-input/test.py +++ b/testsuite/tests/build/hashes/compiler-input/test.py @@ -38,7 +38,7 @@ check_hash(f"version:gnat_external={external_compiler_version()}") # Next, check that selecting a compiler results in it being used # Select the default preferred compiler, in this index is gnat_native=8888 -run_alr("toolchain", "--select", "gnat_native") +run_alr("toolchain", "--select", "gnat_native", "gprbuild") # Clear the build cache so we are able to locate the new hash clear_builds_dir() builds.sync() diff --git a/testsuite/tests/build/hashes/config-types/test.py b/testsuite/tests/build/hashes/config-types/test.py index 69348e12..4ae5724c 100644 --- a/testsuite/tests/build/hashes/config-types/test.py +++ b/testsuite/tests/build/hashes/config-types/test.py @@ -3,7 +3,7 @@ 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.builds import find_hash, hash_input from drivers.asserts import assert_eq from drivers import builds @@ -13,8 +13,8 @@ init_local_crate() alr_with("hello=1.0.1") builds.sync() -# Chech that the hash inputs contains exactly what we expect it to contain - +# Chech that the hash inputs contains exactly what we expect it to contain. +# We cannot know the dependency hash in advance as it depends on the compiler. assert_eq( 'config:hello.var1=true\n' 'config:hello.var2=str\n' @@ -23,6 +23,7 @@ assert_eq( 'config:hello.var5=0\n' 'config:hello.var6=0.00000000000000E+00\n' 'config:hello.var7=0.00000000000000E+00\n' + f'dependency:libhello=1.0.0={find_hash("libhello")}\n' 'profile:hello=RELEASE\n' f'version:gnat_external={external_compiler_version()}\n', hash_input("hello")) diff --git a/testsuite/tests/build/hashes/hashing-inputs/test.py b/testsuite/tests/build/hashes/hashing-inputs/test.py index 8158cc44..56e49fad 100644 --- a/testsuite/tests/build/hashes/hashing-inputs/test.py +++ b/testsuite/tests/build/hashes/hashing-inputs/test.py @@ -3,15 +3,17 @@ Test that the inputs to the hashing properly reflect the build profile and other inputs. """ +import os 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 +from drivers.helpers import content_of run_alr("config", "--set", "--global", "dependencies.shared", "true") init_local_crate() -alr_with("libhello") +alr_with("hello") # Build the crate in default mode, so dependencies are in RELEASE mode run_alr("build") @@ -30,12 +32,12 @@ assert_match(".*profile:libhello=VALIDATION.*", # Check that the hashes are different assert hash1 != hash2, "Hashes should be different" -# Chech that the hash inputs contains exactly what we expect it to contain. +# Check that the hash inputs contains exactly what we expect it to contain. # This includes environment variables, GPR externals set or observed, build # profile, compiler version. assert_eq( - 'config:libhello.var1=true\n' # crate config var + 'config:libhello.var1=false\n' # crate config var (set by hello) '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 @@ -45,4 +47,29 @@ assert_eq( # compiler version hash_input("libhello")) +# Check the hash inputs of a crate with dependencies itself (hello -> libhello). +# We cannot know the dependency hash in advance as it depends on the compiler. +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' + f'dependency:libhello=1.0.0={find_hash("libhello")}\n' + 'profile:hello=VALIDATION\n' + f'version:gnat_external={external_compiler_version()}\n', + hash_input("hello")) + +# Bonus: check the hash inputs for the root crate, used for config regeneration +# Using find_hash here ensures that the hash in the dir name matches the one in +# the inputs of a different crate that depends on it. +assert_eq( + f'dependency:hello=1.0.1={find_hash("hello")}\n' + 'profile:xxx=VALIDATION\n' + f'version:gnat_external={external_compiler_version()}\n', + content_of(os.path.join("alire", "build_hash_inputs")) +) + print("SUCCESS") diff --git a/testsuite/tests/crate_config/no-rebuilds/test.py b/testsuite/tests/crate_config/no-rebuilds/test.py index a6f81aa1..29b8c16a 100644 --- a/testsuite/tests/crate_config/no-rebuilds/test.py +++ b/testsuite/tests/crate_config/no-rebuilds/test.py @@ -16,7 +16,7 @@ assert_match('.*gprbuild: "xxx.*" up to date', p.out) # Switch to another profile and build must happen p = run_alr("build", "--validation", quiet=False) -assert_match('.*\[link\] xxx.adb', p.out) +assert_match('.*\[Ada\]\s+xxx.adb', p.out) # Use same profile, nothing should be recompiled p = run_alr("build", "--validation", quiet=False) -- 2.39.5