From 30da2a90b3d433160c06643fb7ca51722fbae6b8 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 20 Apr 2021 15:36:02 +0200 Subject: Detect environment changes in ad hoc recipes --- libbuild2/adhoc-rule-buildscript.cxx | 38 ++++++++++++++++++++++++------------ libbuild2/bin/init.cxx | 28 ++++++++++++++++++-------- libbuild2/build/script/parser.cxx | 2 +- libbuild2/cc/module.cxx | 5 ++--- libbuild2/functions-process-path.cxx | 1 + libbuild2/functions-process.cxx | 3 ++- libbuild2/types.hxx | 30 ++++++++++++++++++++++------ libbuild2/variable.cxx | 27 ++++++++++++++++++++++--- libbuild2/variable.hxx | 4 ++-- 9 files changed, 101 insertions(+), 37 deletions(-) diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx index dad638e..168856c 100644 --- a/libbuild2/adhoc-rule-buildscript.cxx +++ b/libbuild2/adhoc-rule-buildscript.cxx @@ -229,7 +229,7 @@ namespace build2 timestamp mt (t.load_mtime ()); optional ps; - sha256 pcs, ecs; + sha256 prq_cs, exe_cs, env_cs; { // This is essentially ps=execute_prerequisites(a, t, mt) which we // cannot use because we need to see ad hoc prerequisites. @@ -302,7 +302,7 @@ namespace build2 if (script.depdb_clear) continue; - hash_target (pcs, pt); + hash_target (prq_cs, pt); // The script can reference a program in one of four ways: // @@ -315,11 +315,12 @@ namespace build2 // 4. As a program path/name. // // When it comes to change tracking, there is nothing we can do for - // (4) and there is nothing to do for (3) (assuming builtin semantics - // is stable/backwards-compatible). The (2) case is handled - // automatically by hashing all the variable values referenced by the - // script (see below), which in case of process_path_ex includes the - // checksum, if available. + // (4) (the user can track its environment manually with depdb-env) + // and there is nothing to do for (3) (assuming builtin semantics is + // stable/backwards-compatible). The (2) case is handled automatically + // by hashing all the variable values referenced by the script (see + // below), which in case of process_path_ex includes the checksums + // (both executable and environment), if available. // // This leaves the (1) case, which itself splits into two sub-cases: // the target comes with the dependency information (e.g., imported @@ -336,11 +337,16 @@ namespace build2 // to do; the user may mark tools as ad hoc in order to omit them from // $<). // - if (auto* e = pt.is_a ()) + if (auto* et = pt.is_a ()) { - if (auto* c = e->lookup_metadata ("checksum")) + if (auto* c = et->lookup_metadata ("checksum")) { - ecs.append (*c); + exe_cs.append (*c); + } + + if (auto* e = et->lookup_metadata ("environment")) + { + hash_environment (env_cs, *e); } } } @@ -395,6 +401,9 @@ namespace build2 // Note that this excludes the special $< and $> variables which we // handle below. // + // @@ TODO: maybe detect and decompose process_path_ex in order to + // properly attribute checksum and environment changes? + // if (!script.depdb_clear) { sha256 cs; @@ -436,16 +445,19 @@ namespace build2 if (dd.expect (tcs.string ()) != nullptr) l4 ([&]{trace << "target set change forcing update of " << t;}); - if (dd.expect (pcs.string ()) != nullptr) + if (dd.expect (prq_cs.string ()) != nullptr) l4 ([&]{trace << "prerequisite set change forcing update of " << t;}); } - // Finally the programs checksum. + // Finally the programs and environment checksums. // if (!script.depdb_clear) { - if (dd.expect (ecs.string ()) != nullptr) + if (dd.expect (exe_cs.string ()) != nullptr) l4 ([&]{trace << "program checksum change forcing update of " << t;}); + + if (dd.expect (env_cs.string ()) != nullptr) + l4 ([&]{trace << "environment change forcing update of " << t;}); } const scope* bs (nullptr); diff --git a/libbuild2/bin/init.cxx b/libbuild2/bin/init.cxx index 62c7bcf..7e022fd8 100644 --- a/libbuild2/bin/init.cxx +++ b/libbuild2/bin/init.cxx @@ -724,8 +724,11 @@ namespace build2 } } - rs.assign ("bin.ar.path") = - process_path_ex (ari.ar_path, "ar", ari.ar_checksum); + rs.assign ("bin.ar.path") = process_path_ex ( + ari.ar_path, + "ar", + ari.ar_checksum, + hash_environment (ari.ar_environment)); rs.assign ("bin.ar.id") = ari.ar_id; rs.assign ("bin.ar.signature") = ari.ar_signature; rs.assign ("bin.ar.checksum") = ari.ar_checksum; @@ -744,8 +747,11 @@ namespace build2 if (ranlib != nullptr) { - rs.assign ("bin.ranlib.path") = - process_path_ex (ari.ranlib_path, "ranlib", ari.ranlib_checksum); + rs.assign ("bin.ranlib.path") = process_path_ex ( + ari.ranlib_path, + "ranlib", + ari.ranlib_checksum, + hash_environment (ari.ranlib_environment)); rs.assign ("bin.ranlib.id") = ari.ranlib_id; rs.assign ("bin.ranlib.signature") = ari.ranlib_signature; rs.assign ("bin.ranlib.checksum") = ari.ranlib_checksum; @@ -859,8 +865,11 @@ namespace build2 << " checksum " << ldi.checksum; } - rs.assign ("bin.ld.path") = - process_path_ex (ldi.path, "ld", ldi.checksum); + rs.assign ("bin.ld.path") = process_path_ex ( + ldi.path, + "ld", + ldi.checksum, + hash_environment (ldi.environment)); rs.assign ("bin.ld.id") = ldi.id; rs.assign ("bin.ld.signature") = ldi.signature; rs.assign ("bin.ld.checksum") = ldi.checksum; @@ -996,8 +1005,11 @@ namespace build2 << " checksum " << rci.checksum; } - rs.assign ("bin.rc.path") = - process_path_ex (rci.path, "rc", rci.checksum); + rs.assign ("bin.rc.path") = process_path_ex ( + rci.path, + "rc", + rci.checksum, + hash_environment (rci.environment)); rs.assign ("bin.rc.id") = rci.id; rs.assign ("bin.rc.signature") = rci.signature; rs.assign ("bin.rc.checksum") = rci.checksum; diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx index 9b9f324..4c11688 100644 --- a/libbuild2/build/script/parser.cxx +++ b/libbuild2/build/script/parser.cxx @@ -678,7 +678,7 @@ namespace build2 // if it's process_path_ex, then we may end up with something along // these lines: // - // /usr/bin/g++ name@c++ checksum@deadbeef --version + // /usr/bin/g++ name@c++ checksum@... env-checksum@... --version // // Which is a bit harder to recognize syntactically. So what we are // going to do is have a separate first pass which reduces the diff --git a/libbuild2/cc/module.cxx b/libbuild2/cc/module.cxx index 7ef8cd5..98e0451 100644 --- a/libbuild2/cc/module.cxx +++ b/libbuild2/cc/module.cxx @@ -216,9 +216,8 @@ namespace build2 // Assign values to variables that describe the compiler. // - // @@ TODO: env_checksum. - // - rs.assign (x_path) = process_path_ex (xi.path, x_name, xi.checksum); + rs.assign (x_path) = process_path_ex ( + xi.path, x_name, xi.checksum, env_checksum); const strings& xm (cast (rs.assign (x_mode) = move (mode))); rs.assign (x_id) = xi.id.string (); diff --git a/libbuild2/functions-process-path.cxx b/libbuild2/functions-process-path.cxx index 199ad0d..486a806 100644 --- a/libbuild2/functions-process-path.cxx +++ b/libbuild2/functions-process-path.cxx @@ -28,6 +28,7 @@ namespace build2 f["name"] += &process_path_ex::name; f["checksum"] += &process_path_ex::checksum; + f["env_checksum"] += &process_path_ex::env_checksum; } } } diff --git a/libbuild2/functions-process.cxx b/libbuild2/functions-process.cxx index 4be5149..c18d949 100644 --- a/libbuild2/functions-process.cxx +++ b/libbuild2/functions-process.cxx @@ -217,7 +217,8 @@ namespace build2 size_t erase (0); // This can be a process_path (pair), process_path_ex (process_path - // followed by the name@ and checksum@ pairs), or just a path. + // optionally followed by the name@, checksum@, and env-checksum@ + // pairs), or just a path. // // First, check if the arguments begin with a process_path[_ex] and, if // that's the case, only use the leading name/pair to create the process diff --git a/libbuild2/types.hxx b/libbuild2/types.hxx index 6877f92..dd82ef1 100644 --- a/libbuild2/types.hxx +++ b/libbuild2/types.hxx @@ -293,22 +293,40 @@ namespace build2 // // See also {import,export}.metadata. // + // Note that the environment checksum is calculated in the (potentially + // hermetic) project environment which makes instances of process_path_ex + // project-specific. (We could potentially store the original list of + // environment variables and calculate the checksum on the fly in the + // current context thus making it project-independent. But this will + // complicate things without, currently, much real benefit since all our + // use-cases fit well with the project-specific restriction. We could + // probably even support both variants if desirable.) + // struct process_path_ex: process_path { - optional name; // Stable name for diagnostics. - optional checksum; // Checksum for change tracking. + optional name; // Stable name for diagnostics. + optional checksum; // Executable checksum for change tracking. + optional env_checksum; // Environment checksum for change tracking. using process_path::process_path; - process_path_ex (const process_path& p, string n, optional c = {}) + process_path_ex (const process_path& p, + string n, + optional c = {}, + optional ec = {}) : process_path (p, false /* init */), name (std::move (n)), - checksum (std::move (c)) {} + checksum (std::move (c)), + env_checksum (std::move (ec)) {} - process_path_ex (process_path&& p, string n, optional c = {}) + process_path_ex (process_path&& p, + string n, + optional c = {}, + optional ec = {}) : process_path (std::move (p)), name (std::move (n)), - checksum (std::move (c)) {} + checksum (std::move (c)), + env_checksum (std::move (ec)) {} process_path_ex () = default; }; diff --git a/libbuild2/variable.cxx b/libbuild2/variable.cxx index efda63d..f9e60c1 100644 --- a/libbuild2/variable.cxx +++ b/libbuild2/variable.cxx @@ -1158,10 +1158,19 @@ namespace build2 else if (k == "checksum") { if (!i->simple ()) - throw_invalid_argument (*i, nullptr, "process_path_ex checksum"); + throw_invalid_argument ( + *i, nullptr, "process_path_ex executable checksum"); pp.checksum = move (i->value); } + else if (k == "env-checksum") + { + if (!i->simple ()) + throw_invalid_argument ( + *i, nullptr, "process_path_ex environment checksum"); + + pp.env_checksum = move (i->value); + } else throw invalid_argument ( "unknown key '" + k + "' in process_path_ex value"); @@ -1176,7 +1185,9 @@ namespace build2 auto b (ns.begin ()), i (b), e (ns.end ()); for (i += i->pair ? 2 : 1; i != e && i->pair; i += 2) { - if (!i->simple () || (i->value != "name" && i->value != "checksum")) + if (!i->simple () || (i->value != "name" && + i->value != "checksum" && + i->value != "env-checksum")) break; } @@ -1217,6 +1228,7 @@ namespace build2 lhs.name = move (rhs.name); lhs.checksum = move (rhs.checksum); + lhs.env_checksum = move (rhs.env_checksum); } else { @@ -1224,6 +1236,7 @@ namespace build2 lhs.name = rhs.name; lhs.checksum = rhs.checksum; + lhs.env_checksum = rhs.env_checksum; } } @@ -1252,7 +1265,8 @@ namespace build2 { s.reserve ((x.effect.empty () ? 1 : 2) + (x.name ? 2 : 0) + - (x.checksum ? 2 : 0)); + (x.checksum ? 2 : 0) + + (x.env_checksum ? 2 : 0)); process_path_reverse_impl (x, s); @@ -1269,6 +1283,13 @@ namespace build2 s.back ().pair = '@'; s.push_back (name (*x.checksum)); } + + if (x.env_checksum) + { + s.push_back (name ("env-checksum")); + s.back ().pair = '@'; + s.push_back (name (*x.env_checksum)); + } } return s; diff --git a/libbuild2/variable.hxx b/libbuild2/variable.hxx index d59a891..805b93d 100644 --- a/libbuild2/variable.hxx +++ b/libbuild2/variable.hxx @@ -957,8 +957,8 @@ namespace build2 "insufficient space"); // Represented as a potential @-pair of name(s) corresponding to - // process_path followed by the name@ and checksum@ pairs. So it's a - // container-like. + // process_path optionally followed by the name@, checksum@, and + // env-checksum@ pairs. So it's a container-like. // static process_path_ex convert (names&&); static void assign (value&, process_path_ex&&); -- cgit v1.1