(Feat): Support passing a bazel BuildEventProtocol path #197

TylerJang27 · 2024-12-05T21:32:57Z

Support testing, validating, and uploading from a bazel BuildEventProtocol JSON file

I've added unit and integration tests, and also validated this manually with a local bazel run and a successful upload.

As a follow-up I'll be vendoring the proto crate in this repo

TylerJang27 · 2024-12-05T21:33:58Z

cli/src/main.rs

+        required_unless_present = "bazel_bep_path",
+        conflicts_with = "bazel_bep_path",


This allows it to be mutually exclusive with the --junit-paths as well as --xc-result-path when applicable

TylerJang27 · 2024-12-05T21:34:29Z

cli/src/main.rs

+        required_unless_present = "junit_paths",
+        help = "Path to bazel build event protocol JSON file."
+    )]
+    bazel_bep_path: Option<String>,


If we decide we want to support the binary format too at some point, we can just name that bazel_bep_binary_path. JSON seems the sanest default here

TylerJang27 · 2024-12-05T21:35:11Z

cli/src/main.rs


    log::info!("running command: {:?}", command);
    let run_result = run_test_command(
        &repo,
        &org_url_slug,
        command.first().unwrap(),
        command.iter().skip(1).collect(),
-        junit_paths,
+        &junit_spec,


We can't early parse here like we do in other cases, since we haven't run the test yet 🙃

TylerJang27 · 2024-12-05T21:35:32Z

cli/src/main.rs

+                    let mut parser = BazelBepParser::new(bazel_bep_path.clone());
+                    parser.parse()?;
+                    parser.xml_files()


Not sure if there's a more idiomatic way to do this in rust. Maybe chaining?

Nah, this is pretty good. It gets annoying when having to do mapping with Results

TylerJang27 · 2024-12-05T21:36:54Z

cli/src/upload.rs

@@ -258,6 +273,7 @@ pub async fn run_upload(

    log::info!("Total files pack and upload: {}", file_counter.get_count());
    if file_counter.get_count() == 0 {
+        // DONOTLAND FIX LOG MESSAGE HERE


Will be auditing this to make sure the message still makes sense/works

TylerJang27 · 2024-12-05T21:43:35Z

context/src/bazel_bep/parser.rs

+                    let xml_files = test_result.test_action_output.into_iter().fold(
+                        Vec::new(),
+                        |mut xml_files, action_output| {
+                            if action_output.name.ends_with(".xml") {


test.xml by default afaik, but my understanding is that this may vary for different runners

pv72895

My pre-review - all looks good to me at a first passthroug. Since we're ok with this feature being hacky, I'm ok with believing the external types you're using to be correct / up-to-date enough

cli/src/main.rs

pv72895 · 2024-12-05T23:43:21Z

context/src/bazel_bep/parser.rs

+pub struct TestResult {
+    pub cached: bool,
+    pub xml_files: Vec<String>,
+}


I was under the impression that one test result = one file - is that not the case? Or just being proactive here?

From what I've seen it's just one xml file. But there are multiple files (test.log), and conceivably if you were using bazel to wrap some other invocation, you could get multiple junits, so being proactive

TylerJang27 · 2024-12-06T10:33:56Z

context/src/bazel_bep/parser.rs

+        log::info!(
+            "Parsed {} ({} cached) test results from BEP file",
+            self.test_results.len(),
+            self.test_results.iter().filter(|r| r.cached).count()


Open to input on precomputing this, storing other info, etc. I wanted to strike a balance of only storing info that we found valuable here, while also reporting useful information for debug purposes...

TylerJang27 · 2024-12-06T10:35:38Z

context/test_fixtures/bep_example

These are sanitized BEP files from our repro of the original issue. I have:

Stripped env vars

Simplified paths

Removed extra log output

Cleaned up CLI options

and tried a few different situations to simulate truncation, invalid files, etc.

dfrankland · 2024-12-06T18:47:21Z

cli-tests/Cargo.toml

+# Fork from 0.2.2 that adds serde serialize/deserialize via pbjson
+bazel-bep = { git = "https://github.com/TylerJang27/bazel-bep.git", rev = "e51c546960067b9fe98ae35ae00bc53302973a9e" }


I'd recommend for us to just vendor the crate in the workspace. There's only ~100 lines of code there

Good point, will do as a follow-up

dfrankland · 2024-12-06T18:51:26Z

cli/src/main.rs

+                    let mut parser = BazelBepParser::new(bazel_bep_path.clone());
+                    parser.parse()?;
+                    parser.xml_files()


Nah, this is pretty good. It gets annoying when having to do mapping with Results

cli/src/main.rs

cli/src/runner.rs

context/src/bazel_bep/parser.rs

TylerJang27 · 2024-12-06T23:57:22Z

.trunk/trunk.yaml

These changes didn't end up helping with CI like I'd hoped, but it may prove helpful locally. If need be we can remove later

TylerJang27 · 2024-12-06T23:57:55Z

.github/workflows/pyo3.yml

+            if [ "$(uname -m)" == "aarch64" ]; then
+              curl -L https://github.com/protocolbuffers/protobuf/releases/download/v29.1/protoc-29.1-linux-aarch_64.zip -o protoc.zip
+              unzip protoc.zip
+              export PROTOC=$(pwd)/bin/protoc
+            else
+              curl -L https://github.com/protocolbuffers/protobuf/releases/download/v29.1/protoc-29.1-linux-x86_64.zip -o protoc.zip
+              unzip protoc.zip
+              export PROTOC=$(pwd)/bin/protoc


The package registries were giving me issues. This ended up being a more reliable way to install and pin within the VMs

TylerJang27 · 2024-12-06T23:59:10Z

.github/actions/setup_rust_cargo/action.yaml

+    - name: Install protoc
+      shell: bash
+      run: |
+        if [ -f /opt/homebrew/bin/brew ]; then
+          /opt/homebrew/bin/brew install protobuf
+        elif [ -f /usr/bin/apt ]; then
+          sudo /usr/bin/apt update
+          sudo /usr/bin/apt install -y protobuf-compiler
+        fi
+


The arduino setup action was relentless with rate limiting (not sure why they don't allow bypassing the API call), and I didn't want to pass our whole GH token rn https://github.com/marketplace/actions/setup-protoc

dfrankland · 2024-12-07T00:00:46Z

cli/src/runner.rs

+    let output_paths = match junit_spec {
+        JunitSpec::Paths(paths) => paths,
+        JunitSpec::BazelBep(bep_path) => {
+            let mut parser = BazelBepParser::new(bep_path.clone());


Now that junit_spec is passed by value, we don't need to clone here

dfrankland · 2024-12-07T00:04:30Z

context/src/bazel_bep/parser.rs

+    pub fn print_parsed_results(&self) {
+        if !self.errors.is_empty() {
+            log::warn!("Errors parsing BEP file: {:?}", &self.errors);
+        }
+
+        let (test_count, cached_count) = self.test_results.iter().fold(
+            (0, 0),
+            |(mut test_count, mut cached_count), test_result| {
+                test_count += test_result.xml_files.len();
+                if test_result.cached {
+                    cached_count += test_result.xml_files.len();
+                }
+                (test_count, cached_count)
+            },
+        );
+        log::info!(
+            "Parsed {} ({} cached) test results from BEP file",
+            test_count,
+            cached_count
+        );
+    }


Optional: I would leave the log::info for the CLI crate. This method could just return the number of tests and number of cached tests to be used however the caller pleases

Ah, fair point. Will handle in follow-up

TylerJang27 commented Dec 5, 2024

View reviewed changes

pv72895 reviewed Dec 5, 2024

View reviewed changes

TylerJang27 commented Dec 6, 2024

View reviewed changes

TylerJang27 force-pushed the tyler/trunk-13743-handle-being-provided-a-path-to-bep-file-in-cli branch from f6f2969 to 32bc6ef Compare December 6, 2024 10:43

TylerJang27 requested a review from dfrankland December 6, 2024 10:44

TylerJang27 marked this pull request as ready for review December 6, 2024 10:44

TylerJang27 requested review from tenesttang-trunk, max-trunk and pv72895 December 6, 2024 10:44

TylerJang27 force-pushed the tyler/trunk-13743-handle-being-provided-a-path-to-bep-file-in-cli branch from 32bc6ef to f72c26a Compare December 6, 2024 18:42

dfrankland reviewed Dec 6, 2024

View reviewed changes

TylerJang27 added 7 commits December 6, 2024 12:43

first impl working

7db9ca9

mutually exclusive test

ce7a508

lock

7326dbd

address most of first round comments

65e39d9

extract prints

74f5e25

cached count

3d1ecdb

install protoc in ci

08eed55

TylerJang27 force-pushed the tyler/trunk-13743-handle-being-provided-a-path-to-bep-file-in-cli branch from f72c26a to 08eed55 Compare December 6, 2024 21:57

TylerJang27 added 8 commits December 6, 2024 14:01

pin version

5073157

try to install with trunk

4407f53

apt install

3cb1c05

sudo

d895474

add missing python setup for vm

026182a

try grabbing from release

1b385d8

install unzip

b0b8b58

fix unzip

2855541

TylerJang27 requested a review from dfrankland December 6, 2024 23:55

TylerJang27 commented Dec 6, 2024

View reviewed changes

dfrankland approved these changes Dec 7, 2024

View reviewed changes

TylerJang27 merged commit 104f48c into main Dec 7, 2024
13 checks passed

This was referenced Dec 7, 2024

(Chore): Cleanup bazel-bep usage #205

Merged

(Chore): Build protoc from source #208

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Feat): Support passing a bazel BuildEventProtocol path #197

(Feat): Support passing a bazel BuildEventProtocol path #197

TylerJang27 commented Dec 5, 2024 •

edited

Loading

TylerJang27 Dec 5, 2024

TylerJang27 Dec 5, 2024

TylerJang27 Dec 5, 2024

TylerJang27 Dec 5, 2024

dfrankland Dec 6, 2024

TylerJang27 Dec 5, 2024

TylerJang27 Dec 5, 2024

pv72895 left a comment

pv72895 Dec 5, 2024

TylerJang27 Dec 6, 2024

TylerJang27 Dec 6, 2024

TylerJang27 Dec 6, 2024 •

edited

Loading

dfrankland Dec 6, 2024

TylerJang27 Dec 6, 2024

dfrankland Dec 6, 2024

TylerJang27 Dec 6, 2024

TylerJang27 Dec 6, 2024

TylerJang27 Dec 6, 2024

dfrankland Dec 7, 2024

dfrankland Dec 7, 2024 •

edited

Loading

TylerJang27 Dec 7, 2024

		required_unless_present = "bazel_bep_path",
		conflicts_with = "bazel_bep_path",

		# Fork from 0.2.2 that adds serde serialize/deserialize via pbjson
		bazel-bep = { git = "https://github.com/TylerJang27/bazel-bep.git", rev = "e51c546960067b9fe98ae35ae00bc53302973a9e" }

(Feat): Support passing a bazel BuildEventProtocol path #197

(Feat): Support passing a bazel BuildEventProtocol path #197

Conversation

TylerJang27 commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pv72895 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerJang27 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfrankland Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerJang27 commented Dec 5, 2024 •

edited

Loading

TylerJang27 Dec 6, 2024 •

edited

Loading

dfrankland Dec 7, 2024 •

edited

Loading