From 0d4ea3bf50f3addb239ff8998c05fa4b367c6c9f Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Mon, 28 Nov 2022 10:18:57 +0300 Subject: Make sure we only build one package config per interactive tennant --- doc/manual.cli | 13 +++ libbrep/build-package.hxx | 2 +- libbrep/package.hxx | 2 +- libbrep/package.xml | 267 +++++++++++++++++++++++----------------------- load/load.cxx | 28 +++-- mod/mod-build-task.cxx | 149 +++++++++++++++++++++++++- 6 files changed, 310 insertions(+), 151 deletions(-) diff --git a/doc/manual.cli b/doc/manual.cli index d0a1923..53dcb28 100644 --- a/doc/manual.cli +++ b/doc/manual.cli @@ -359,8 +359,21 @@ being applied. Currently, only the following value groups can be overridden: \ build-email build-{warning,error}-email builds build-{include,exclude} +*-builds *-build-{include,exclude} \ +Note that the build constraints group values (both common and build package +configuration-specific) are overridden hierarchically so that the +\c{[\b{*-}]\b{build-}{\b{include},\b{exclude}\}} overrides don't affect the +respective \c{[\b{*-}]\b{builds}} values. + +Note also that the common and build package configuration-specific build +constraints group value overrides are mutually exclusive. If the common build +constraints are overridden, then all the configuration-specific constraints +are removed. Otherwise, if any configuration-specific constraints are +overridden, then for the remaining configurations the build constraints are +reset to \cb{builds:\ none}. + See \l{bpkg#manifest-package Package Manifest} for details on these values. diff --git a/libbrep/build-package.hxx b/libbrep/build-package.hxx index 79fb6a4..6ca8702 100644 --- a/libbrep/build-package.hxx +++ b/libbrep/build-package.hxx @@ -25,7 +25,7 @@ namespace brep // // Foreign object that is mapped to a subset of the tenant object. // - #pragma db object table("build_tenant") pointer(shared_ptr) readonly + #pragma db object table("build_tenant") pointer(shared_ptr) class build_tenant { public: diff --git a/libbrep/package.hxx b/libbrep/package.hxx index 4a68a07..cf6ae64 100644 --- a/libbrep/package.hxx +++ b/libbrep/package.hxx @@ -18,7 +18,7 @@ // Used by the data migration entries. // -#define LIBBREP_PACKAGE_SCHEMA_VERSION_BASE 25 +#define LIBBREP_PACKAGE_SCHEMA_VERSION_BASE 26 #pragma db model version(LIBBREP_PACKAGE_SCHEMA_VERSION_BASE, 26, closed) diff --git a/libbrep/package.xml b/libbrep/package.xml index 4e5544a..8f32284 100644 --- a/libbrep/package.xml +++ b/libbrep/package.xml @@ -1,139 +1,5 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + @@ -986,6 +852,137 @@
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
diff --git a/load/load.cxx b/load/load.cxx index 000743c..ed48910 100644 --- a/load/load.cxx +++ b/load/load.cxx @@ -368,7 +368,8 @@ load_packages (const shared_ptr& rp, const repository_location& cl, database& db, bool ignore_unknown, - const manifest_name_values& overrides) + const manifest_name_values& overrides, + const string& overrides_name) { // packages_timestamp other than timestamp_nonexistent signals the // repository packages are already loaded. @@ -434,15 +435,17 @@ load_packages (const shared_ptr& rp, { if (rp->internal) { + if (!overrides.empty ()) try { - pm.override (overrides, "" /* name */); + pm.override (overrides, overrides_name); } - catch (const manifest_parsing&) + catch (const manifest_parsing& e) { - // Overrides are already validated (see below). - // - assert (false); + cerr << "error: unable to override " << p << " manifest: " << e + << endl; + + throw failed (); } // Create internal package object. @@ -580,8 +583,7 @@ load_packages (const shared_ptr& rp, move (ts), move (pm.builds), move (pm.build_constraints), - build_package_configs (make_move_iterator (pm.build_configs.begin ()), - make_move_iterator (pm.build_configs.end ())), + move (pm.build_configs), move (pm.location), move (pm.fragment), move (pm.sha256sum), @@ -979,7 +981,8 @@ load_repositories (const options& lo, !pr->cache_location.empty () ? pr->cache_location : cl, db, ignore_unknown, - manifest_name_values () /* overrides */); + manifest_name_values () /* overrides */, + "" /* overrides_name */); load_repositories (lo, pr, @@ -1453,6 +1456,10 @@ try // Parse and validate overrides, if specified. // + // Note that here we make sure that the overrides manifest is valid. + // Applying overrides to a specific package manifest may still fail (see + // package_manifest::validate_overrides() for details). + // manifest_name_values overrides; if (ops.overrides_file_specified ()) @@ -1579,7 +1586,8 @@ try r->cache_location, db, ops.ignore_unknown (), - overrides); + overrides, + ops.overrides_file ().string ()); } // On the second pass over the internal repositories we load their diff --git a/mod/mod-build-task.cxx b/mod/mod-build-task.cxx index a15ff90..30bff0d 100644 --- a/mod/mod-build-task.cxx +++ b/mod/mod-build-task.cxx @@ -182,7 +182,7 @@ handle (request& rq, response& rs) dir_path () /* start */, path_match_flags::match_absent)) conf_machines.emplace (build_target_config_id {c.target, c.name}, - config_machine {&c, &m}); + config_machine {&c, &m}); } catch (const invalid_path&) {} } @@ -603,9 +603,9 @@ handle (request& rq, response& rs) // configurations that have already been acted upon (initially empty). // // This is why we query the database for configurations that should not be - // built (in the built state, or in the building state and not - // expired). Having such a list we will select the first build - // configuration that is not in the list (if available) for the response. + // built (in the built state, or in the building state and not expired). + // Having such a list we will select the first build configuration that is + // not in the list (if available) for the response. // using bld_query = query; using prep_bld_query = prepared_query; @@ -726,6 +726,139 @@ handle (request& rq, response& rs) shared_ptr p (build_db_->load (id)); + // Note that a request to interactively build a package in multiple + // configurations is most likely a mistake than a deliberate choice. + // Thus, for the interactive tenant let's check if the package can be + // built in multiple configurations. If that's the case then we will + // put all the potential builds into the aborted state and continue + // iterating looking for another package. Otherwise, just proceed for + // this package normally. + // + // It also feels like a good idea to archive an interactive tenant + // after a build object is created for it, regardless if the build + // task is issued or not. This way we make sure that an interactive + // build is never performed multiple times for such a tenant for any + // reason (multiple toolchains, buildtab change, etc). Note that the + // build result will still be accepted for an archived build. + // + shared_ptr t (build_db_->load (id.tenant)); + + if (t->interactive) + { + // Note that the tenant can be archived via some other package on + // some previous iteration. Skip the package if that's the case. + // + if (t->archived) + continue; + + // Collect the potential build configurations as all combinations of + // the tenant's packages build configurations and the non-excluded + // (by the packages) build target configurations. Note that here we + // ignore the machines from the task request. + // + struct build_config + { + package_id pid; + string pc; + const build_target_config* tc; + }; + + small_vector build_configs; + + // Note that we don't bother creating a prepared query here, since + // its highly unlikely to encounter multiple interactive tenants per + // task request. Given that we archive such tenants immediately, as + // a common case there will be none. + // + pkg_query pq (pkg_query::build_tenant::id == t->id); + for (auto& tp: build_db_->query (pq)) + { + shared_ptr p ( + build_db_->load (tp.id)); + + for (build_package_config& pc: p->configs) + { + for (const auto& tc: *target_conf_) + { + if (!exclude (pc, p->builds, p->constraints, tc)) + build_configs.push_back (build_config {p->id, pc.name, &tc}); + } + } + } + + // If multiple build configurations are collected, then abort all + // the potential builds and continue iterating over the packages. + // + if (build_configs.size () > 1) + { + // Abort the builds. + // + for (build_config& c: build_configs) + { + const string& pc (c.pc); + const build_target_config& tc (*c.tc); + + build_id bid (c.pid, + tc.target, + tc.name, + pc, + tqm.toolchain_name, + toolchain_version); + + // Can there be any existing builds for such a tenant? Doesn't + // seem so, unless due to some manual intervention into the + // database. Anyway, let's just leave such a build alone. + // + shared_ptr b (build_db_->find (bid)); + + if (b == nullptr) + { + b = make_shared ( + move (bid.package.tenant), + move (bid.package.name), + bp.version, + move (bid.target), + move (bid.target_config_name), + move (bid.package_config_name), + move (bid.toolchain_name), + toolchain_version, + nullopt /* interactive */, + nullopt /* agent_fp */, + nullopt /* agent_challenge */, + "brep" /* machine */, + "build task module" /* machine_summary */, + "" /* controller_checksum */, + "" /* machine_checksum */); + + b->state = build_state::built; + b->status = result_status::abort; + + b->soft_timestamp = b->timestamp; + b->hard_timestamp = b->soft_timestamp; + + // Mark the section as loaded, so results are updated. + // + b->results_section.load (); + + b->results.push_back ( + operation_result { + "configure", + result_status::abort, + "error: multiple configurations for interactive build\n"}); + + build_db_->persist (b); + } + } + + // Archive the tenant. + // + t->archived = true; + build_db_->update (t); + + continue; // Skip the package. + } + } + for (build_package_config& pc: p->configs) { pkg_config_name = pc.name; @@ -870,6 +1003,14 @@ handle (request& rq, response& rs) build_db_->update (b); } + // Archive an interactive tenant. + // + if (t->interactive) + { + t->archived = true; + build_db_->update (t); + } + // Finally, prepare the task response manifest. // tsm = task (move (b), move (p), move (pc), move (t), cm); -- cgit v1.1