diff options
| -rw-r--r-- | etc/brep-module.conf | 13 | ||||
| -rw-r--r-- | etc/private/install/brep-module.conf | 13 | ||||
| -rw-r--r-- | libbrep/package.cxx | 2 | ||||
| -rw-r--r-- | libbrep/package.hxx | 45 | ||||
| -rw-r--r-- | libbrep/package.xml | 8 | ||||
| -rw-r--r-- | libbrep/review-manifest.cxx | 220 | ||||
| -rw-r--r-- | libbrep/review-manifest.hxx | 80 | ||||
| -rw-r--r-- | load/.gitignore | 1 | ||||
| -rw-r--r-- | load/buildfile | 6 | ||||
| -rw-r--r-- | load/load-with-metadata.in | 130 | ||||
| -rw-r--r-- | load/load.cli | 27 | ||||
| -rw-r--r-- | load/load.cxx | 404 | ||||
| -rw-r--r-- | load/types-parsers.cxx | 7 | ||||
| -rw-r--r-- | load/types-parsers.hxx | 7 | ||||
| -rw-r--r-- | mod/mod-package-details.cxx | 12 | ||||
| -rw-r--r-- | mod/mod-package-version-details.cxx | 16 | ||||
| -rw-r--r-- | mod/module.cli | 21 | ||||
| -rw-r--r-- | mod/page.cxx | 74 | ||||
| -rw-r--r-- | mod/page.hxx | 43 | ||||
| -rw-r--r-- | tests/manifest/buildfile | 6 | ||||
| -rw-r--r-- | tests/manifest/driver.cxx | 59 | ||||
| -rw-r--r-- | tests/manifest/review.testscript | 171 | ||||
| -rw-r--r-- | www/package-details-body.css | 7 | ||||
| -rw-r--r-- | www/package-version-details-body.css | 18 |
24 files changed, 1362 insertions, 28 deletions
diff --git a/etc/brep-module.conf b/etc/brep-module.conf index d5a5e78..3963379 100644 --- a/etc/brep-module.conf +++ b/etc/brep-module.conf @@ -297,6 +297,19 @@ menu About=?about # bindist-url +# The base URL for the reviews manifest files. If this option is specified, +# then the review information is displayed on the package version details +# page. +# +# The complete URL is formed by adding the following path to the base: +# +# <project>/<package>/<version>/reviews.manifest +# +# Note that no separator is added between the base and this path. +# +# reviews-url + + # The openssl program to be used for crypto operations. You can also specify # additional options that should be passed to the openssl program with # openssl-option. If the openssl program is not explicitly specified, then brep diff --git a/etc/private/install/brep-module.conf b/etc/private/install/brep-module.conf index bfaa8f6..bad5ede 100644 --- a/etc/private/install/brep-module.conf +++ b/etc/private/install/brep-module.conf @@ -297,6 +297,19 @@ menu About=?about # bindist-url +# The base URL for the reviews manifest files. If this option is specified, +# then the review information is displayed on the package version details +# page. +# +# The complete URL is formed by adding the following path to the base: +# +# <project>/<package>/<version>/reviews.manifest +# +# Note that no separator is added between the base and this path. +# +# reviews-url + + # The openssl program to be used for crypto operations. You can also specify # additional options that should be passed to the openssl program with # openssl-option. If the openssl program is not explicitly specified, then brep diff --git a/libbrep/package.cxx b/libbrep/package.cxx index 4eb6fe8..391a583 100644 --- a/libbrep/package.cxx +++ b/libbrep/package.cxx @@ -84,6 +84,7 @@ namespace brep build_auxiliaries_type ac, package_build_bot_keys bk, package_build_configs bcs, + optional<reviews_summary> rvs, optional<path> lc, optional<string> fr, optional<string> sh, @@ -119,6 +120,7 @@ namespace brep build_auxiliaries (move (ac)), build_bot_keys (move (bk)), build_configs (move (bcs)), + reviews (move (rvs)), internal_repository (move (rp)), location (move (lc)), fragment (move (fr)), diff --git a/libbrep/package.hxx b/libbrep/package.hxx index c06d8d3..668dc5c 100644 --- a/libbrep/package.hxx +++ b/libbrep/package.hxx @@ -20,7 +20,7 @@ // #define LIBBREP_PACKAGE_SCHEMA_VERSION_BASE 34 -#pragma db model version(LIBBREP_PACKAGE_SCHEMA_VERSION_BASE, 34, closed) +#pragma db model version(LIBBREP_PACKAGE_SCHEMA_VERSION_BASE, 35, closed) namespace brep { @@ -224,9 +224,8 @@ namespace brep // certificate // #pragma db value - class certificate + struct certificate { - public: string fingerprint; // SHA256 fingerprint. Note: foreign-mapped in build. string name; // CN component of Subject. string organization; // O component of Subject. @@ -536,6 +535,35 @@ namespace brep #pragma db member(package_build_bot_key_key::outer) column("config_index") #pragma db member(package_build_bot_key_key::inner) column("index") + // Number of the passed and failed reviews and the path to the + // reviews.manifest file this information comes form. The path is relative + // to the root of the package metadata directory. + // + #pragma db value + struct reviews_summary + { + // May not be both zero. + // + size_t pass; + size_t fail; + + path manifest_file; + }; + + inline bool + operator== (const reviews_summary& x, const reviews_summary& y) + { + return x.pass == y.pass && + x.fail == y.fail && + x.manifest_file == y.manifest_file; + } + + inline bool + operator!= (const reviews_summary& x, const reviews_summary& y) + { + return !(x == y); + } + // Tweak package_id mapping to include a constraint (this only affects the // database schema). // @@ -589,6 +617,7 @@ namespace brep build_auxiliaries_type, package_build_bot_keys, package_build_configs, + optional<reviews_summary>, optional<path> location, optional<string> fragment, optional<string> sha256sum, @@ -691,6 +720,9 @@ namespace brep // odb::section build_section; + optional<reviews_summary> reviews; + odb::section reviews_section; + // Note that it is foreign-mapped in build. // lazy_shared_ptr<repository_type> internal_repository; @@ -916,8 +948,11 @@ namespace brep id_column("") key_column("") value_column("key_") value_not_null \ section(unused_section) - #pragma db member(build_section) load(lazy) update(always) - #pragma db member(unused_section) load(lazy) update(manual) + #pragma db member(reviews) section(reviews_section) + + #pragma db member(build_section) load(lazy) update(always) + #pragma db member(reviews_section) load(lazy) update(always) + #pragma db member(unused_section) load(lazy) update(manual) // other_repositories // diff --git a/libbrep/package.xml b/libbrep/package.xml index 42cad89..8b6c706 100644 --- a/libbrep/package.xml +++ b/libbrep/package.xml @@ -1,4 +1,12 @@ <changelog xmlns="http://www.codesynthesis.com/xmlns/odb/changelog" database="pgsql" schema-name="package" version="1"> + <changeset version="35"> + <alter-table name="package"> + <add-column name="reviews_pass" type="BIGINT" null="true"/> + <add-column name="reviews_fail" type="BIGINT" null="true"/> + <add-column name="reviews_manifest_file" type="TEXT" null="true"/> + </alter-table> + </changeset> + <model version="34"> <table name="tenant" kind="object"> <column name="id" type="TEXT" null="false"/> diff --git a/libbrep/review-manifest.cxx b/libbrep/review-manifest.cxx new file mode 100644 index 0000000..3592e69 --- /dev/null +++ b/libbrep/review-manifest.cxx @@ -0,0 +1,220 @@ +// file : libbrep/review-manifest.cxx -*- C++ -*- +// license : MIT; see accompanying LICENSE file + +#include <libbrep/review-manifest.hxx> + +#include <libbutl/manifest-parser.hxx> +#include <libbutl/manifest-serializer.hxx> + +using namespace std; +using namespace butl; + +namespace brep +{ + using parser = manifest_parser; + using parsing = manifest_parsing; + using serializer = manifest_serializer; + using serialization = manifest_serialization; + using name_value = manifest_name_value; + + // review_result + // + string + to_string (review_result r) + { + switch (r) + { + case review_result::pass: return "pass"; + case review_result::fail: return "fail"; + case review_result::unchanged: return "unchanged"; + } + + assert (false); + return string (); + } + + review_result + to_review_result (const string& r) + { + if (r == "pass") return review_result::pass; + else if (r == "fail") return review_result::fail; + else if (r == "unchanged") return review_result::unchanged; + else throw invalid_argument ("invalid review result '" + r + '\''); + } + + // review_manifest + // + review_manifest:: + review_manifest (parser& p, bool iu) + : review_manifest (p, p.next (), iu) + { + // Make sure this is the end. + // + name_value nv (p.next ()); + if (!nv.empty ()) + throw parsing (p.name (), nv.name_line, nv.name_column, + "single review manifest expected"); + } + + review_manifest:: + review_manifest (parser& p, name_value nv, bool iu) + { + auto bad_name ([&p, &nv](const string& d) { + throw parsing (p.name (), nv.name_line, nv.name_column, d);}); + + auto bad_value ([&p, &nv](const string& d) { + throw parsing (p.name (), nv.value_line, nv.value_column, d);}); + + // Make sure this is the start and we support the version. + // + if (!nv.name.empty ()) + throw parsing (p.name (), nv.name_line, nv.name_column, + "start of review manifest expected"); + + if (nv.value != "1") + throw parsing (p.name (), nv.value_line, nv.value_column, + "unsupported format version"); + + bool need_base (false); + bool need_details (false); + + for (nv = p.next (); !nv.empty (); nv = p.next ()) + { + string& n (nv.name); + string& v (nv.value); + + if (n == "reviewed-by") + { + if (!reviewed_by.empty ()) + bad_name ("reviewer redefinition"); + + if (v.empty ()) + bad_value ("empty reviewer"); + + reviewed_by = move (v); + } + else if (n.size () > 7 && n.compare (0, 7, "result-") == 0) + { + string name (n, 7, n.size () - 7); + + if (find_if (results.begin (), results.end (), + [&name] (const review_aspect& r) + { + return name == r.name; + }) != results.end ()) + bad_name (name + " review result redefinition"); + + try + { + review_result r (to_review_result (v)); + + if (r == review_result::fail) + need_details = true; + + if (r == review_result::unchanged) + need_base = true; + + results.push_back (review_aspect {move (name), r}); + } + catch (const invalid_argument& e) + { + bad_value (e.what ()); + } + } + else if (n == "base-version") + { + if (base_version) + bad_name ("base version redefinition"); + + try + { + base_version = bpkg::version (v); + } + catch (const invalid_argument& e) + { + bad_value (e.what ()); + } + } + else if (n == "details-url") + { + if (details_url) + bad_name ("details url redefinition"); + + try + { + details_url = url (v); + } + catch (const invalid_argument& e) + { + bad_value (e.what ()); + } + } + else if (!iu) + bad_name ("unknown name '" + n + "' in review manifest"); + } + + // Verify all non-optional values were specified. + // + if (reviewed_by.empty ()) + bad_value ("no reviewer specified"); + + if (results.empty ()) + bad_value ("no result specified"); + + if (!base_version && need_base) + bad_value ("no base version specified"); + + if (!details_url && need_details) + bad_value ("no details url specified"); + } + + void review_manifest:: + serialize (serializer& s) const + { + // @@ Should we check that all non-optional values are specified and all + // values are valid? + // + s.next ("", "1"); // Start of manifest. + + auto bad_value ([&s](const string& d) { + throw serialization (s.name (), d);}); + + if (reviewed_by.empty ()) + bad_value ("empty reviewer"); + + s.next ("reviewed-by", reviewed_by); + + for (const review_aspect& r: results) + s.next ("result-" + r.name, to_string (r.result)); + + if (base_version) + s.next ("base-version", base_version->string ()); + + if (details_url) + s.next ("details-url", details_url->string ()); + + s.next ("", ""); // End of manifest. + } + + // review_manifests + // + review_manifests:: + review_manifests (parser& p, bool iu) + { + // Parse review manifests. + // + for (name_value nv (p.next ()); !nv.empty (); nv = p.next ()) + emplace_back (p, move (nv), iu); + } + + void review_manifests:: + serialize (serializer& s) const + { + // Serialize review manifests. + // + for (const review_manifest& m: *this) + m.serialize (s); + + s.next ("", ""); // End of stream. + } +} diff --git a/libbrep/review-manifest.hxx b/libbrep/review-manifest.hxx new file mode 100644 index 0000000..260fdec --- /dev/null +++ b/libbrep/review-manifest.hxx @@ -0,0 +1,80 @@ +// file : libbrep/review-manifest.hxx -*- C++ -*- +// license : MIT; see accompanying LICENSE file + +#ifndef LIBBREP_REVIEW_MANIFEST_HXX +#define LIBBREP_REVIEW_MANIFEST_HXX + +#include <libbutl/manifest-forward.hxx> + +#include <libbpkg/manifest.hxx> + +#include <libbrep/types.hxx> +#include <libbrep/utility.hxx> + +namespace brep +{ + enum class review_result: uint8_t + { + pass, + fail, + unchanged + }; + + string + to_string (review_result); + + review_result + to_review_result (const string&); // May throw invalid_argument. + + inline ostream& + operator<< (ostream& os, review_result r) + { + return os << to_string (r); + } + + struct review_aspect + { + string name; // code, build, test, doc, etc + review_result result; + }; + + class review_manifest + { + public: + string reviewed_by; + vector<review_aspect> results; + optional<bpkg::version> base_version; + optional<url> details_url; + + review_manifest (string r, + vector<review_aspect> rs, + optional<bpkg::version> bv, + optional<url> u) + : reviewed_by (move (r)), + results (move (rs)), + base_version (move (bv)), + details_url (move (u)) {} + + public: + review_manifest () = default; + review_manifest (butl::manifest_parser&, bool ignore_unknown = false); + review_manifest (butl::manifest_parser&, + butl::manifest_name_value start, + bool ignore_unknown = false); + + void + serialize (butl::manifest_serializer&) const; + }; + + class review_manifests: public vector<review_manifest> + { + public: + review_manifests () = default; + review_manifests (butl::manifest_parser&, bool ignore_unknown = false); + + void + serialize (butl::manifest_serializer&) const; + }; +} + +#endif // LIBBREP_REVIEW_MANIFEST_HXX diff --git a/load/.gitignore b/load/.gitignore index 035e847..314cf8f 100644 --- a/load/.gitignore +++ b/load/.gitignore @@ -1,2 +1,3 @@ *-options.?xx brep-load +brep-load-with-metadata diff --git a/load/buildfile b/load/buildfile index 4278f20..51b374d 100644 --- a/load/buildfile +++ b/load/buildfile @@ -6,11 +6,17 @@ import libs += libodb-pgsql%lib{odb-pgsql} import libs += libbutl%lib{butl} import libs += libbpkg%lib{bpkg} +import mods = bpkg-util%bash{utility} + include ../libbrep/ +./: exe{brep-load} exe{brep-load-with-metadata} + exe{brep-load}: {hxx ixx cxx}{* -load-options} {hxx ixx cxx}{load-options} \ ../libbrep/lib{brep} $libs +exe{brep-load-with-metadata}: in{load-with-metadata} $mods + # Build options. # obj{load}: cxx.poptions += -DBREP_COPYRIGHT=\"$copyright\" diff --git a/load/load-with-metadata.in b/load/load-with-metadata.in new file mode 100644 index 0000000..a99709e --- /dev/null +++ b/load/load-with-metadata.in @@ -0,0 +1,130 @@ +#!/usr/bin/env bash + +# file : load/load-with-metadata.in +# license : MIT; see accompanying LICENSE file + +# The wrapper around brep-load, which pulls the package metadata from a git +# repository and runs brep-load, passing the metadata directory to it. +# +# Specifically, pull a pre-cloned (read-only) git repository with the contents +# of an archive-based bpkg repository. Run brep-load with the `--metadata +# <dir>/owners` option, the --metadata-changed option, if the current snapshot +# of the repository has not yet been processed by brep-load, and forward any +# further arguments to brep-load. +# +# --timeout <seconds> +# +# Git operation timeout. Specifically, the operation will be aborted if +# there is no network activity for the specified time. Default is 60 +# seconds. Note that currently the git timeout is only supported for the +# http(s) transport. +# +# --brep-load <path> +# +# The brep-load program to be used. This should be the path to the brep-load +# executable. +# +# Note also that this script maintains the <dir>.load file which contains the +# last successfully processed commit. +# +usage="usage: $0 [<options>] <dir> [<brep-load-args>]" + +owd="$(pwd)" +trap "{ cd '$owd'; exit 1; }" ERR +set -o errtrace # Trap in functions and subshells. +set -o pipefail # Fail if any pipeline command fails. +shopt -s lastpipe # Execute last pipeline command in the current shell. +shopt -s nullglob # Expand no-match globs to nothing rather than themselves. + +@import bpkg-util/utility@ # check_git_connectivity() + +# The script's own options. +# +timeout=60 +brep_load= + +while [[ "$#" -gt 0 ]]; do + case "$1" in + --timeout) + shift + timeout="$1" + shift || true + ;; + --brep-load) + shift + brep_load="$1" + shift || true + ;; + *) + break + ;; + esac +done + +# The repository directory. +# +repo_dir="${1%/}" + +# Validate options and arguments. +# +if [[ -z "$repo_dir" ]]; then + error "$usage" +fi + +if [[ ! -d "$repo_dir" ]]; then + error "'$repo_dir' does not exist or is not a directory" +fi + +shift # repo_dir + +# If brep-load path is not specified, then use the brep-load program from the +# script directory, if present. Otherwise, use the 'brep-load' path. +# +if [[ -z "$brep_load" ]]; then + brep_load="$(dirname "$(realpath "${BASH_SOURCE[0]}")")/brep-load" + + if [[ ! -x "$brep_load" ]]; then + brep_load=brep-load + fi +fi + +# Make sure the commit file is present. +# +load_commit="$repo_dir.load" +touch "$load_commit" + +# Pull the repository. +# +if ! remote_url="$(git -C "$repo_dir" config --get remote.origin.url)"; then + error "'$repo_dir' is not a git repository" +fi + +# Git doesn't support the connection timeout option. The options we use are +# just an approximation of the former, that, in particular, don't cover the +# connection establishing. To work around this problem, before running a git +# command that assumes the remote repository communication we manually check +# connectivity with the remote repository. +# +check_git_connectivity "$remote_url" "$timeout" + +# Fail if no network activity happens during the time specified. +# +git -c http.lowSpeedLimit=1 -c "http.lowSpeedTime=$timeout" \ + -C "$repo_dir" pull -q + +# Match the HEAD commit id to the one stored in the file. If it matches, then +# nothing changed in the repository since it has been processed by brep-load +# last time and we should not pass the --metadata-changed option to brep-load. +# +commit="$(git -C "$repo_dir" rev-parse HEAD)" +pc="$(cat "$load_commit")" + +loader_options=(--metadata "$repo_dir/owners") + +if [[ "$commit" != "$pc" ]]; then + loader_options+=(--metadata-changed) +fi + +"$brep_load" "${loader_options[@]}" "$@" + +echo "$commit" >"$load_commit" diff --git a/load/load.cli b/load/load.cli index bda186a..fbdfbd8 100644 --- a/load/load.cli +++ b/load/load.cli @@ -126,6 +126,29 @@ class options \cb{--service-id} option to be specified." }; + brep::dir_path --metadata + { + "<dir>", + "Directory where the package metadata manifest files are located. If + specified, then (re-)load the metadata if the package information is also + (re-)loaded or update it if the \cb{--metadata-changed} option is + specified. + + The subdirectory hierarchy under this directory is expected to be in the + following form: + + \ + <project>/<package>/<version>/ + \ + " + } + + bool --metadata-changed + { + "Update the package metadata even if the package information is not + reloaded." + }; + brep::path --overrides-file { "<file>", @@ -186,7 +209,7 @@ class options this option to specify multiple package manager options." } - brep::path openssl = "openssl" + brep::path --openssl = "openssl" { "<path>", "The openssl program to be used for crypto operations. You can also @@ -195,7 +218,7 @@ class options specified, then \cb{brep-load} will use \cb{openssl} by default." } - brep::strings openssl-option + brep::strings --openssl-option { "<opt>", "Additional option to be passed to the openssl program (see \cb{openssl} diff --git a/load/load.cxx b/load/load.cxx index 2b2cd56..f79b606 100644 --- a/load/load.cxx +++ b/load/load.cxx @@ -3,6 +3,7 @@ #include <signal.h> // signal() +#include <map> #include <cerrno> #include <chrono> #include <thread> // this_thread::sleep_for() @@ -31,6 +32,7 @@ #include <libbrep/package.hxx> #include <libbrep/package-odb.hxx> #include <libbrep/database-lock.hxx> +#include <libbrep/review-manifest.hxx> #include <load/load-options.hxx> #include <load/options-types.hxx> @@ -56,6 +58,7 @@ static const char* help_info ( static const path packages ("packages.manifest"); static const path repositories ("repositories.manifest"); +static const path reviews ("reviews.manifest"); // Retry executing bpkg on recoverable errors for about 10 seconds. // @@ -362,6 +365,56 @@ repository_info (const options& lo, const string& rl, const cstrings& options) } } +// Map of package versions to their metadata information in the form it is +// stored in the database (reviews summary, etc). +// +// This map is filled by recursively traversing the metadata directory and +// parsing the encountered metadata manifest files (reviews.manifest, etc; see +// --metadata option for background on metadata). Afterwards, this map is used +// as a data source for the being persisted/updated package objects. +// +struct package_version_key +{ + package_name name; + brep::version version; + + package_version_key (package_name n, brep::version v) + : name (move (n)), version (move (v)) {} + + bool + operator< (const package_version_key& k) const + { + if (int r = name.compare (k.name)) + return r < 0; + + return version < k.version; + } +}; + +class package_version_metadata +{ +public: + // Extracted from the package metadata directory. Must match the respective + // package manifest information. + // + package_name project; + + optional<reviews_summary> reviews; + + // The directory the metadata manifest files are located. It has the + // <project>/<package>/<version> form and is only used for diagnostics. + // + dir_path + directory () const + { + assert (reviews); // At least one kind of metadata must be present. + return reviews->manifest_file.directory (); + } +}; + +using package_version_metadata_map = std::map<package_version_key, + package_version_metadata>; + // Load the repository packages from the packages.manifest file and persist // the repository. Should be called once per repository. // @@ -372,7 +425,8 @@ load_packages (const options& lo, database& db, bool ignore_unknown, const manifest_name_values& overrides, - const string& overrides_name) + const string& overrides_name, + optional<package_version_metadata_map>& metadata) { // packages_timestamp other than timestamp_nonexistent signals the // repository packages are already loaded. @@ -728,6 +782,31 @@ load_packages (const options& lo, keys_to_objects (move (pm.build_configs[i].bot_keys)); } + optional<reviews_summary> rvs; + + if (metadata) + { + auto i (metadata->find (package_version_key {pm.name, pm.version})); + + if (i != metadata->end ()) + { + package_version_metadata& md (i->second); + + if (md.project != project) + { + cerr << "error: project '" << project << "' of package " + << pm.name << ' ' << pm.version << " doesn't match " + << "metadata directory path " + << lo.metadata () / md.directory (); + + throw failed (); + } + + if (md.reviews) + rvs = move (md.reviews); + } + } + p = make_shared<package> ( move (pm.name), move (pm.version), @@ -758,6 +837,7 @@ load_packages (const options& lo, move (pm.build_auxiliaries), move (bot_keys), move (build_configs), + move (rvs), move (pm.location), move (pm.fragment), move (pm.sha256sum), @@ -1153,13 +1233,16 @@ load_repositories (const options& lo, // We don't apply overrides to the external packages. // + optional<package_version_metadata_map> metadata; + load_packages (lo, pr, !pr->cache_location.empty () ? pr->cache_location : cl, db, ignore_unknown, manifest_name_values () /* overrides */, - "" /* overrides_name */); + "" /* overrides_name */, + metadata); load_repositories (lo, pr, @@ -1778,6 +1861,11 @@ try } } + // Note: the interactive tenant implies private. + // + if (ops.interactive_specified ()) + ops.private_ (true); + // Parse and validate overrides, if specified. // // Note that here we make sure that the overrides manifest is valid. @@ -1818,34 +1906,236 @@ try ops.db_port (), "options='-c default_transaction_isolation=serializable'"); + // Load the description of all the internal repositories from the + // configuration file. + // + internal_repositories irs (load_repositories (path (argv[1]))); + // Prevent several brep utility instances from updating the package database // simultaneously. // database_lock l (db); - transaction t (db.begin ()); - - // Check that the package database schema matches the current one. + // Check that the package database schema matches the current one and if the + // package information needs to be (re-)loaded. // - const string ds ("package"); - if (schema_catalog::current_version (db, ds) != db.schema_version (ds)) + bool load_pkgs; { - cerr << "error: package database schema differs from the current one" - << endl << " info: use brep-migrate to migrate the database" << endl; - throw failed (); + transaction t (db.begin ()); + + // Check the database schema match. + // + const string ds ("package"); + + if (schema_catalog::current_version (db, ds) != db.schema_version (ds)) + { + cerr << "error: package database schema differs from the current one" + << endl << " info: use brep-migrate to migrate the database" << endl; + throw failed (); + } + + load_pkgs = (ops.force () || changed (tnt, irs, db)); + + t.commit (); } - // Note: the interactive tenant implies private. + // Check if the package versions metadata needs to be (re-)loaded and, if + // that's the case, stash it in the memory. // - if (ops.interactive_specified ()) - ops.private_ (true); + optional<package_version_metadata_map> metadata; + if (ops.metadata_specified () && (load_pkgs || ops.metadata_changed ())) + { + metadata = package_version_metadata_map (); - // Load the description of all the internal repositories from the - // configuration file. + const dir_path& d (ops.metadata ()); + + // The first level are package projects. + // + try + { + for (const dir_entry& e: dir_iterator (d, dir_iterator::no_follow)) + { + const string& n (e.path ().string ()); + + if (e.type () != entry_type::directory || n[0] == '.') + continue; + + package_name project; + + try + { + project = package_name (n); + } + catch (const invalid_argument& e) + { + cerr << "error: name of subdirectory '" << n << "' in " << d + << " is not a project name: " << e << endl; + throw failed (); + } + + // The second level are package names. + // + dir_path pd (d / path_cast<dir_path> (e.path ())); + + try + { + for (const dir_entry& e: dir_iterator (pd, dir_iterator::no_follow)) + { + const string& n (e.path ().string ()); + + if (e.type () != entry_type::directory || n[0] == '.') + continue; + + package_name name; + + try + { + name = package_name (n); + } + catch (const invalid_argument& e) + { + cerr << "error: name of subdirectory '" << n << "' in " << pd + << " is not a package name: " << e << endl; + throw failed (); + } + + // The third level are package versions. + // + dir_path vd (pd / path_cast<dir_path> (e.path ())); + + try + { + for (const dir_entry& e: dir_iterator (vd, + dir_iterator::no_follow)) + { + const string& n (e.path ().string ()); + + if (e.type () != entry_type::directory || n[0] == '.') + continue; + + version ver; + + try + { + ver = version (n); + } + catch (const invalid_argument& e) + { + cerr << "error: name of subdirectory '" << n << "' in " << vd + << " is not a package version: " << e << endl; + throw failed (); + } + + dir_path md (vd / path_cast<dir_path> (e.path ())); + + // Parse the reviews.manifest file, if present. + // + // Note that semantically, the absent manifest file and the + // empty manifest list are equivalent and result in an absent + // reviews summary. + // + optional<reviews_summary> rs; + { + path rf (md / reviews); + + try + { + if (file_exists (rf)) + { + ifdstream ifs (rf); + manifest_parser mp (ifs, rf.string ()); + + // Count the passed and failed reviews. + // + size_t ps (0); + size_t fl (0); + + for (review_manifest& m: + review_manifests (mp, ops.ignore_unknown ())) + { + bool fail (false); + + for (const review_aspect& r: m.results) + { + switch (r.result) + { + case review_result::fail: fail = true; break; + + case review_result::unchanged: + { + cerr << "error: unsupported review result " + << "'unchanged' in " << rf << endl; + throw failed (); + } + + case review_result::pass: break; // Noop + } + } + + ++(fail ? fl : ps); + } + + if (ps + fl != 0) + rs = reviews_summary {ps, fl, rf.relative (d)}; + } + } + catch (const manifest_parsing& e) + { + cerr << "error: unable to parse reviews: " << e << endl; + throw failed (); + } + catch (const io_error& e) + { + cerr << "error: unable to read " << rf << ": " << e << endl; + throw failed (); + } + catch (const system_error& e) + { + cerr << "error: unable to stat " << rf << ": " << e << endl; + throw failed (); + } + } + + // Add the package version metadata to the map if any kind of + // metadata is present. + // + if (rs) + { + (*metadata)[package_version_key {name, move (ver)}] = + package_version_metadata {project, move (rs)}; + } + } + } + catch (const system_error& e) + { + cerr << "error: unable to iterate over " << vd << ": " << e + << endl; + throw failed (); + } + } + } + catch (const system_error& e) + { + cerr << "error: unable to iterate over " << pd << ": " << e << endl; + throw failed (); + } + } + } + catch (const system_error& e) + { + cerr << "error: unable to iterate over " << d << ": " << e << endl; + throw failed (); + } + } + + // Bail out if no package information nor metadata needs to be loaded. // - internal_repositories irs (load_repositories (path (argv[1]))); + if (!load_pkgs && !metadata) + return 0; + + transaction t (db.begin ()); - if (ops.force () || changed (tnt, irs, db)) + if (load_pkgs) { shared_ptr<tenant> t; // Not NULL in the --existing-tenant mode. @@ -2015,7 +2305,8 @@ try db, ops.ignore_unknown (), overrides, - ops.overrides_file ().string ()); + ops.overrides_file ().string (), + metadata); } // On the second pass over the internal repositories we load their @@ -2070,6 +2361,83 @@ try } } } + else if (metadata) + { + // Iterate over the packages which contain metadata and apply the changes, + // if present. Erase the metadata map entries which introduce such + // changes, so at the end only the newly added metadata is left in the + // map. + // + using query = query<package>; + + for (package& p: db.query<package> (query::reviews.pass.is_not_null ())) + { + bool u (false); + auto i (metadata->find (package_version_key {p.name, p.version})); + + if (i == metadata->end ()) + { + // Mark the section as loaded, so the reviews summary is updated. + // + p.reviews_section.load (); + p.reviews = nullopt; + u = true; + } + else + { + package_version_metadata& md (i->second); + + if (md.project != p.project) + { + cerr << "error: project '" << p.project << "' of package " + << p.name << ' ' << p.version << " doesn't match metadata " + << "directory path " << ops.metadata () / md.directory (); + + throw failed (); + } + + db.load (p, p.reviews_section); + + if (p.reviews != md.reviews) + { + p.reviews = move (md.reviews); + u = true; + } + + metadata->erase (i); + } + + if (u) + db.update (p); + } + + // Add the newly added metadata to the packages. + // + for (auto& m: *metadata) + { + if (shared_ptr<package> p = + db.find<package> (package_id (tnt, m.first.name, m.first.version))) + { + package_version_metadata& md (m.second); + + if (m.second.project != p->project) + { + cerr << "error: project '" << p->project << "' of package " + << p->name << ' ' << p->version << " doesn't match metadata " + << "directory path " << ops.metadata () / md.directory (); + + throw failed (); + } + + // Mark the section as loaded, so the reviews summary is updated. + // + p->reviews_section.load (); + p->reviews = move (md.reviews); + + db.update (p); + } + } + } t.commit (); return 0; diff --git a/load/types-parsers.cxx b/load/types-parsers.cxx index a18330d..4f031df 100644 --- a/load/types-parsers.cxx +++ b/load/types-parsers.cxx @@ -40,6 +40,13 @@ namespace cli parse_path (x, s); } + void parser<dir_path>:: + parse (dir_path& x, bool& xs, scanner& s) + { + xs = true; + parse_path (x, s); + } + void parser<ignore_unresolved_conditional_dependencies>:: parse (ignore_unresolved_conditional_dependencies& x, bool& xs, scanner& s) { diff --git a/load/types-parsers.hxx b/load/types-parsers.hxx index fcf5113..b79cca4 100644 --- a/load/types-parsers.hxx +++ b/load/types-parsers.hxx @@ -26,6 +26,13 @@ namespace cli }; template <> + struct parser<brep::dir_path> + { + static void + parse (brep::dir_path&, bool&, scanner&); + }; + + template <> struct parser<brep::ignore_unresolved_conditional_dependencies> { static void diff --git a/mod/mod-package-details.cxx b/mod/mod-package-details.cxx index 1fb51da..15a4115 100644 --- a/mod/mod-package-details.cxx +++ b/mod/mod-package-details.cxx @@ -270,8 +270,16 @@ handle (request& rq, response& rs) // s << TR_REPOSITORY (rl, root, tenant) << TR_DEPENDS (p->dependencies, root, tenant) - << TR_REQUIRES (p->requirements) - << ~TBODY + << TR_REQUIRES (p->requirements); + + if (options_->reviews_url_specified ()) + { + package_db_->load (*p, p->reviews_section); + + s << TR_REVIEWS_SUMMARY (p->reviews, options_->reviews_url ()); + } + + s << ~TBODY << ~TABLE; } s << ~DIV; diff --git a/mod/mod-package-version-details.cxx b/mod/mod-package-version-details.cxx index 91923e5..e28310c 100644 --- a/mod/mod-package-version-details.cxx +++ b/mod/mod-package-version-details.cxx @@ -528,6 +528,22 @@ handle (request& rq, response& rs) print_tests (test_dependency_type::examples); print_tests (test_dependency_type::benchmarks); + if (options_->reviews_url_specified ()) + { + package_db_->load (*pkg, pkg->reviews_section); + + const optional<reviews_summary>& rvs (pkg->reviews); + const string& u (options_->reviews_url ()); + + s << H3 << "Reviews" << ~H3 + << TABLE(CLASS="proplist", ID="reviews") + << TBODY + << TR_REVIEWS_COUNTER (review_result::fail, rvs, u) + << TR_REVIEWS_COUNTER (review_result::pass, rvs, u) + << ~TBODY + << ~TABLE; + } + bool builds (build_db_ != nullptr && pkg->buildable); if (builds) diff --git a/mod/module.cli b/mod/module.cli index 5133935..7b0c0d2 100644 --- a/mod/module.cli +++ b/mod/module.cli @@ -454,6 +454,25 @@ namespace brep } }; + class package_version_metadata + { + string reviews-url + { + "<url>", + "The base URL for the reviews manifest files. If this option is + specified, then the review information is displayed on the package + version details page. + + The complete URL is formed by adding the following path to the base: + + \ + <project>/<package>/<version>/reviews.manifest + \ + + Note that no separator is added between the base and this path." + } + }; + class page { web::xhtml::fragment logo @@ -530,6 +549,7 @@ namespace brep search, page, repository_url, + package_version_metadata, handler { }; @@ -538,6 +558,7 @@ namespace brep build, build_db, page, repository_url, + package_version_metadata, handler { dir_path bindist-root diff --git a/mod/page.cxx b/mod/page.cxx index 177fb64..17fef91 100644 --- a/mod/page.cxx +++ b/mod/page.cxx @@ -618,6 +618,80 @@ namespace brep << ~TR; } + // TR_REVIEWS_SUMMARY + // + void TR_REVIEWS_SUMMARY:: + operator() (serializer& s) const + { + s << TR(CLASS="reviews") + << TH << "reviews" << ~TH + << TD + << SPAN(CLASS="value"); + + if (reviews_) + { + s << A + << HREF + << reviews_url_ << reviews_->manifest_file + << ~HREF; + + if (reviews_->fail != 0) + s << SPAN(CLASS="fail") << '-' << reviews_->fail << ~SPAN; + + if (reviews_->fail != 0 && reviews_->pass != 0) + s << '/'; + + if (reviews_->pass != 0) + s << SPAN(CLASS="pass") << '+' << reviews_->pass << ~SPAN; + + s << ~A; + } + else + s << SPAN(CLASS="none") << 0 << ~SPAN; + + s << ~SPAN + << ~TD + << ~TR; + } + + // TR_REVIEWS_COUNTER + // + void TR_REVIEWS_COUNTER:: + operator() (serializer& s) const + { + const char* l (result == review_result::fail ? "fail" : "pass"); + + s << TR(CLASS=l) + << TH << l << ~TH + << TD + << SPAN(CLASS="value"); + + if (reviews_) + { + size_t n (result == review_result::fail + ? reviews_->fail + : reviews_->pass); + + if (n != 0) + { + s << A + << HREF + << reviews_url_ << reviews_->manifest_file + << ~HREF + << SPAN(CLASS=l) << n << ~SPAN + << ~A; + } + else + s << n; + } + else + s << SPAN(CLASS="none") << 0 << ~SPAN; + + s << ~SPAN + << ~TD + << ~TR; + } + // TR_URL // void TR_URL:: diff --git a/mod/page.hxx b/mod/page.hxx index 7329e2d..3455fe8 100644 --- a/mod/page.hxx +++ b/mod/page.hxx @@ -15,6 +15,7 @@ #include <libbrep/build.hxx> #include <libbrep/package.hxx> +#include <libbrep/review-manifest.hxx> // review_result #include <mod/diagnostics.hxx> #include <mod/options-types.hxx> // page_menu @@ -371,6 +372,48 @@ namespace brep const requirements& requirements_; }; + // Generate package versions reviews summary element. + // + class TR_REVIEWS_SUMMARY + { + public: + TR_REVIEWS_SUMMARY (const optional<reviews_summary>& rs, const string& u) + : reviews_ (rs), reviews_url_ (u) {} + + void + operator() (xml::serializer&) const; + + private: + const optional<reviews_summary>& reviews_; + const string& reviews_url_; + }; + + // Generate package versions reviews summary counter element. The passed + // review result denotes which kind of counter needs to be displayed and can + // only be fail or pass. + // + class TR_REVIEWS_COUNTER + { + public: + TR_REVIEWS_COUNTER (review_result r, + const optional<reviews_summary>& rs, + const string& u) + : result (r), + reviews_ (rs), + reviews_url_ (u) + { + assert (r == review_result::fail || r == review_result::pass); + } + + void + operator() (xml::serializer&) const; + + private: + review_result result; + const optional<reviews_summary>& reviews_; + const string& reviews_url_; + }; + // Generate url element. Strip the `<scheme>://` prefix from the link text. // class TR_URL diff --git a/tests/manifest/buildfile b/tests/manifest/buildfile new file mode 100644 index 0000000..e6f5a85 --- /dev/null +++ b/tests/manifest/buildfile @@ -0,0 +1,6 @@ +# file : tests/manifest/buildfile +# license : MIT; see accompanying LICENSE file + +import libs = lib{brep} + +exe{driver}: {hxx cxx}{*} $libs testscript{*} diff --git a/tests/manifest/driver.cxx b/tests/manifest/driver.cxx new file mode 100644 index 0000000..5c70ea5 --- /dev/null +++ b/tests/manifest/driver.cxx @@ -0,0 +1,59 @@ +// file : tests/manifest/driver.cxx -*- C++ -*- +// license : MIT; see accompanying LICENSE file + +#include <ios> // ios_base::failbit, ios_base::badbit +#include <iostream> + +#include <libbutl/utility.hxx> // operator<<(ostream,exception) +#include <libbutl/manifest-parser.hxx> +#include <libbutl/manifest-serializer.hxx> + +#include <libbrep/review-manifest.hxx> + +#undef NDEBUG +#include <cassert> + +using namespace std; +using namespace butl; +using namespace brep; + +// Usage: argv[0] (-r | -rl) +// +// Read and parse manifest from STDIN and serialize it to STDOUT. The +// following options specify the manifest type. +// +// -r parse review manifest +// -rl parse review manifest list +// +int +main (int argc, char* argv[]) +try +{ + assert (argc == 2); + string opt (argv[1]); + + cin.exceptions (ios_base::failbit | ios_base::badbit); + cout.exceptions (ios_base::failbit | ios_base::badbit); + + manifest_parser p (cin, "stdin"); + manifest_serializer s (cout, "stdout"); + + if (opt == "-r") + review_manifest (p).serialize (s); + else if (opt == "-rl") + review_manifests (p).serialize (s); + else + assert (false); + + return 0; +} +catch (const manifest_parsing& e) +{ + cerr << e << endl; + return 1; +} +catch (const manifest_serialization& e) +{ + cerr << e << endl; + return 1; +} diff --git a/tests/manifest/review.testscript b/tests/manifest/review.testscript new file mode 100644 index 0000000..e3daa66 --- /dev/null +++ b/tests/manifest/review.testscript @@ -0,0 +1,171 @@ +# file : tests/manifest/review.testscript +# license : MIT; see accompanying LICENSE file + +: single-manifest +: +{ + test.options += -r + + : valid + : + : Roundtrip the review manifest. + : + { + $* <<EOF >>EOF + : 1 + reviewed-by: John Doe <john@doe.com> + result-code: pass + result-build: fail + result-doc: unchanged + base-version: 1.0.2+3 + details-url: https://example.com/issues/1 + EOF + } + + : unknown-name + : + { + $* <<EOI 2>"stdin:2:1: error: unknown name 'unknown-name' in review manifest" != 0 + : 1 + unknown-name: John Doe <john@doe.com> + EOI + } + + : redefinition + : + { + : reviewed-by + : + { + $* <<EOI 2>"stdin:3:1: error: reviewer redefinition" != 0 + : 1 + reviewed-by: John Doe <john@doe.com> + reviewed-by: John Doe <john@doe.com> + EOI + } + + : result-code + : + { + $* <<EOI 2>"stdin:3:1: error: code review result redefinition" != 0 + : 1 + result-code: pass + result-code: fail + EOI + } + } + + : invalid + : + { + : reviewed-by-empty + : + { + $* <<EOI 2>"stdin:2:13: error: empty reviewer" != 0 + : 1 + reviewed-by: + EOI + } + + : result-code + : + { + $* <<EOI 2>"stdin:2:14: error: invalid review result 'fails'" != 0 + : 1 + result-code: fails + EOI + } + + : details-url + : + { + $* <<EOI 2>"stdin:2:13: error: empty URL" != 0 + : 1 + details-url: + EOI + } + } + + : mandatory + : + { + : reviewed-by + : + { + $* <<EOI 2>"stdin:2:1: error: no reviewer specified" != 0 + : 1 + EOI + } + + : no-result + : + { + $* <<EOI 2>"stdin:3:1: error: no result specified" != 0 + : 1 + reviewed-by: John Doe <john@doe.com> + EOI + } + + : no-base-version + : + { + $* <<EOI 2>"stdin:4:1: error: no base version specified" != 0 + : 1 + reviewed-by: John Doe <john@doe.com> + result-code: unchanged + EOI + } + + : no-details-url + : + { + $* <<EOI 2>"stdin:4:1: error: no details url specified" != 0 + : 1 + reviewed-by: John Doe <john@doe.com> + result-code: fail + EOI + } + } +} + +: multiple-manifests +: +{ + test.options += -rl + + : valid-manifest-list + : + : Roundtrip the review manifests list. + : + { + $* <<EOF >>EOF + : 1 + reviewed-by: John Doe <john@doe.com> + result-code: pass + : + reviewed-by: John Doe <john@doe.com> + result-build: pass + EOF + } + + : empty-manifest-list + : + : Roundtrip the empty manifests list. + : + { + $* <:'' >:'' + } + + : no-details-url + : + { + $* <<EOI 2>"stdin:7:1: error: no details url specified" != 0 + : 1 + reviewed-by: John Doe <john@doe.com> + result-build: pass + : + reviewed-by: John Doe <john@doe.com> + result-code: fail + EOI + } +} diff --git a/www/package-details-body.css b/www/package-details-body.css index 1083c54..bdef845 100644 --- a/www/package-details-body.css +++ b/www/package-details-body.css @@ -185,7 +185,8 @@ table.version th {width: 7.6em;} table.version tr.version td .value, table.version tr.priority td .value, table.version tr.depends td .value, -table.version tr.requires td .value +table.version tr.requires td .value, +table.version tr.reviews td .value { /* <code> style. */ font-family: monospace; @@ -195,3 +196,7 @@ table.version tr.requires td .value table.version tr.priority td .security {color: #ff0000; font-weight: bold;} table.version tr.priority td .high {color: #ff0000;} table.version tr.priority td .medium {color: #fe7c04;} + +table.version tr.reviews td .none {color: #fe7c04;} +table.version tr.reviews td .fail {color: #ff0000;} +table.version tr.reviews td .pass {color: #00bb00;} diff --git a/www/package-version-details-body.css b/www/package-version-details-body.css index 1c41ed5..284ccc2 100644 --- a/www/package-version-details-body.css +++ b/www/package-version-details-body.css @@ -289,6 +289,24 @@ h1, h2, h3 } /* + * Reviews. + */ +#reviews {margin-top: .4em; margin-bottom: 1em;} +#reviews th {width: 5.7em;} + +#reviews tr.fail td .value, +#reviews tr.pass td .value +{ + /* <code> style. */ + font-family: monospace; + font-size: 0.94em; +} + +#reviews tr td .value .none {color: #fe7c04;} +#reviews tr td .value .fail {color: #ff0000;} +#reviews tr td .value .pass {color: #00bb00;} + +/* * Binaries. */ #binaries |