From 4f2159a262a7eeb20bfdff909367dd38eaafd7c4 Mon Sep 17 00:00:00 2001 From: Julian Squires Date: Tue, 3 Jan 2023 14:49:06 -0330 Subject: [PATCH 1/7] mzbuild: use stable coverage option instrument-coverage has been stable since Rust 1.60. I am removing the unresolved symbols warning hack for Nix because, like the commentors who closed the reference issues, it's unclear to me what would cause this, and I'd rather resolve it by fixing the actual issue. Looking at related issues like rust-lang/rust#92769, it's probably an artifact of the implementation in nightly, which hopefully is fixed now that it's stable. --- misc/python/materialize/mzbuild.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/misc/python/materialize/mzbuild.py b/misc/python/materialize/mzbuild.py index 02adf20675c25..08f6f0cdae9bf 100644 --- a/misc/python/materialize/mzbuild.py +++ b/misc/python/materialize/mzbuild.py @@ -231,14 +231,8 @@ def __init__(self, rd: RepositoryDetails, path: Path, config: Dict[str, Any]): self.channel = None if rd.coverage: self.rustflags += [ - "-Zinstrument-coverage", - # Nix generates some unresolved symbols that -Zinstrument-coverage - # somehow causes the linker to complain about, so just disable - # warnings about unresolved symbols and hope it all works out. - # See: https://github.com/nix-rust/nix/issues/1116 - "-Clink-arg=-Wl,--warn-unresolved-symbols", + "-Cinstrument-coverage", ] - self.channel = "nightly" if len(self.bins) == 0 and len(self.examples) == 0: raise ValueError("mzbuild config is missing pre-build target") From b6f5599b45cc5491b36402f9b2a6eddb2cdebf2e Mon Sep 17 00:00:00 2001 From: Julian Squires Date: Tue, 10 Jan 2023 09:58:44 -0330 Subject: [PATCH 2/7] Ignore profiling files generated by -Cinstrument-coverage --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 3b14cd8c953f7..f401f3686d831 100644 --- a/.gitignore +++ b/.gitignore @@ -29,6 +29,8 @@ junit_*.xml perf.data* **/*.rej **/*.orig +**/*.profraw +**/*.profdata # This is un-orthodox, but adding it to the repo would "tie" it to each users # local version of nixpkgs. This way, we can all use the flake and have a From 40747b3edb88009c35d2fc7d728dabee95bcb7c2 Mon Sep 17 00:00:00 2001 From: Julian Squires Date: Tue, 7 Feb 2023 17:22:47 -0330 Subject: [PATCH 3/7] ci: install llvm-tools-preview in the ci-builder image Necessary to run rust-profdata and rust-cov for working with coverage data. --- bin/ci-builder | 2 +- ci/builder/Dockerfile | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/bin/ci-builder b/bin/ci-builder index f8c09eb8397b5..4921c248ab205 100755 --- a/bin/ci-builder +++ b/bin/ci-builder @@ -60,7 +60,7 @@ arch_go=$(arch_go "$arch_gcc") cid_file=ci/builder/.${channel%%-*}.cidfile -rust_components=rustc,cargo,rust-std-$arch_gcc-unknown-linux-gnu +rust_components=rustc,cargo,rust-std-$arch_gcc-unknown-linux-gnu,llvm-tools-preview if [[ $rust_version = nightly ]]; then rust_components+=,miri-preview else diff --git a/ci/builder/Dockerfile b/ci/builder/Dockerfile index e06f6b5494f7c..92cb7525cc678 100644 --- a/ci/builder/Dockerfile +++ b/ci/builder/Dockerfile @@ -158,7 +158,8 @@ RUN mkdir rust \ && cargo install --root /usr/local --version "=0.9.44" cargo-nextest \ && `: Until https://github.com/est31/cargo-udeps/pull/147 is released in cargo-udeps` \ && cargo install --root /usr/local --git https://github.com/est31/cargo-udeps --rev=b84d478ef3efd7264dba8c15c31a50c6399dc5bb --locked cargo-udeps --features=vendored-openssl \ - && cargo install --root /usr/local --version "=0.2.15" --no-default-features --features=s3,openssl/vendored sccache + && cargo install --root /usr/local --version "=0.2.15" --no-default-features --features=s3,openssl/vendored sccache \ + && cargo install --root /usr/local --version "=0.3.6" cargo-binutils # Link the system lld into the cross-compiling sysroot. From 6284a9b93363edd652da17f8b471783384b59f41 Mon Sep 17 00:00:00 2001 From: Julian Squires Date: Thu, 9 Feb 2023 16:43:08 -0330 Subject: [PATCH 4/7] mzcompose: don't duplicate coverage configuration Without this, we end up mutating the same config again, which then causes docker compose to fail with: services.sqllogictest.environment array items[1,2] must be unique --- misc/python/materialize/mzcompose/__init__.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/misc/python/materialize/mzcompose/__init__.py b/misc/python/materialize/mzcompose/__init__.py index f4fbe4fa5fd4d..3d7d01bd22d68 100644 --- a/misc/python/materialize/mzcompose/__init__.py +++ b/misc/python/materialize/mzcompose/__init__.py @@ -185,16 +185,19 @@ def _munge_services( if "allow_host_ports" in config: config.pop("allow_host_ports") - if self.repo.rd.coverage: + coverage_volume = "./coverage:/coverage" + if self.repo.rd.coverage and coverage_volume not in config.get( + "volumes", [] + ): # Emit coverage information to a file in a directory that is # bind-mounted to the "coverage" directory on the host. We # inject the configuration to all services for simplicity, but # this only have an effect if the service runs instrumented Rust # binaries. + config.setdefault("volumes", []).append(coverage_volume) config.setdefault("environment", []).append( f"LLVM_PROFILE_FILE=/coverage/{name}-%m.profraw" ) - config.setdefault("volumes", []).append("./coverage:/coverage") # Determine mzbuild specs and inject them into services accordingly. deps = self.repo.resolve_dependencies(images) From eb951094d42e80be45264ef9656db14b44a5f08a Mon Sep 17 00:00:00 2001 From: Julian Squires Date: Wed, 22 Mar 2023 15:27:54 -0230 Subject: [PATCH 5/7] mzcompose: use multiple concurrent profraw files to avoid collisions The goal here is to avoid sqllogictest and clusterd from overwriting each-other's profraw outputs. %p is by PID, which one would expect to be sufficient, and %Nm allows up to 9 concurrent profiles for the same build-ID; perhaps the two together is overkill, but this seems like a reasonable starting point. --- misc/python/materialize/mzcompose/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/misc/python/materialize/mzcompose/__init__.py b/misc/python/materialize/mzcompose/__init__.py index 3d7d01bd22d68..9172f518925e6 100644 --- a/misc/python/materialize/mzcompose/__init__.py +++ b/misc/python/materialize/mzcompose/__init__.py @@ -196,7 +196,7 @@ def _munge_services( # binaries. config.setdefault("volumes", []).append(coverage_volume) config.setdefault("environment", []).append( - f"LLVM_PROFILE_FILE=/coverage/{name}-%m.profraw" + f"LLVM_PROFILE_FILE=/coverage/{name}-%p-%9m.profraw" ) # Determine mzbuild specs and inject them into services accordingly. From cccca2559def8aa1a6a7438c409e5e213929df14 Mon Sep 17 00:00:00 2001 From: Julian Squires Date: Tue, 3 Jan 2023 14:56:53 -0330 Subject: [PATCH 6/7] ci: re-enable coverage in nightly We can now use the stable Rust toolchain and hopefully this no longer suffers from the aforementioned OOMs. Just fast sqllogictests for now, with coverage in the raw JSON format until we figure out what we want to do with it. --- ci/nightly/coverage.py | 62 ++++++++++++++++++++++++++++------------- ci/nightly/pipeline.yml | 4 +-- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/ci/nightly/coverage.py b/ci/nightly/coverage.py index a2a0bf07ec9ba..5fdf61652a3bf 100644 --- a/ci/nightly/coverage.py +++ b/ci/nightly/coverage.py @@ -19,48 +19,70 @@ pipeline.template.yml and the docstring on `trim_pipeline` below. """ +import subprocess import sys from pathlib import Path -import yaml - import materialize.cli.mzcompose def main() -> int: - with open(Path(__file__).parent.parent / "test" / "pipeline.template.yml") as f: - pipeline = yaml.safe_load(f) - - tests = [] - - for step in pipeline["steps"]: - for plugin in step.get("plugins", []): - for plugin_name, plugin_config in plugin.items(): - if plugin_name == "./ci/plugins/mzcompose": - tests.append((plugin_config["composition"], plugin_config["run"])) + # Just the fast sqllogictests, for now + tests = [("sqllogictest", "default")] for (composition, workflow) in tests: - print(f"==> Running workflow {workflow} in {composition}") materialize.cli.mzcompose.main( [ - "run", - workflow, - "--coverage", "--find", composition, + "--coverage", + "run", + workflow, ] ) materialize.cli.mzcompose.main( [ - "down", - "-v", - "--coverage", "--find", composition, + "down", + "-v", ] ) - # TODO: gather and combine coverage information. + # NB: mzcompose _munge_services() sets LLVM_PROFILE_FILE, so that + # output will go to a special coverage volume, but as a + # conesquence we can't really distinguish between sqllogictest and + # clusterd's output. + subprocess.run( + [ + "rust-profdata", + "merge", + "-sparse", + *Path("test/sqllogictest/coverage").glob("sqllogictest*.profraw"), + "-o", + "sqllogictest.profdata", + ] + ) + with open("coverage-sqllogictest.json", "w") as out: + subprocess.run( + [ + "rust-cov", + "export", + "./target-xcompile/x86_64-unknown-linux-gnu/release/sqllogictest", + "--instr-profile=sqllogictest.profdata", + ], + stdout=out, + ) + with open("coverage-clusterd.json", "w") as out: + subprocess.run( + [ + "rust-cov", + "export", + "./target-xcompile/x86_64-unknown-linux-gnu/release/clusterd", + "--instr-profile=sqllogictest.profdata", + ], + stdout=out, + ) return 0 diff --git a/ci/nightly/pipeline.yml b/ci/nightly/pipeline.yml index 3116ef3bfd4c1..1f410bdc12148 100644 --- a/ci/nightly/pipeline.yml +++ b/ci/nightly/pipeline.yml @@ -97,10 +97,10 @@ steps: - id: coverage label: Code coverage timeout_in_minutes: 240 - command: bin/ci-builder run nightly bin/pyactivate -m ci.nightly.coverage + command: bin/ci-builder run stable bin/pyactivate -m ci.nightly.coverage + artifact_paths: coverage-*.json agents: queue: linux-x86_64 - skip: Disabled due to persistent OOMs when linking - id: kafka-matrix label: Kafka smoke test against previous Kafka versions From 402003bfaed1668c78d3060394ebee18c123661f Mon Sep 17 00:00:00 2001 From: Julian Squires Date: Wed, 22 Mar 2023 17:51:10 -0230 Subject: [PATCH 7/7] bin/run: add a --coverage argument So that one can conveniently locally run environmentd or sqllogictest with coverage. --- misc/python/materialize/cli/run.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/misc/python/materialize/cli/run.py b/misc/python/materialize/cli/run.py index 3026cb4709f14..964c2e1ac0e7a 100644 --- a/misc/python/materialize/cli/run.py +++ b/misc/python/materialize/cli/run.py @@ -114,6 +114,11 @@ def main() -> int: help="Disables the limited codesigning we do on macOS to support Instruments", action="store_true", ) + parser.add_argument( + "--coverage", + help="Build with coverage", + action="store_true", + ) args = parser.parse_intermixed_args() # Handle `+toolchain` like rustup. @@ -262,6 +267,8 @@ def _build(args: argparse.Namespace, extra_programs: list[str] = []) -> int: if args.tokio_console: features += ["tokio-console"] env["RUSTFLAGS"] = env.get("RUSTFLAGS", "") + " --cfg=tokio_unstable" + if args.coverage: + env["RUSTFLAGS"] = env.get("RUSTFLAGS", "") + " -Cinstrument-coverage" if args.features: features.extend(args.features.split(",")) if features: