Skip to content

fix: proto inclusion#2515

Merged
tac0turtle merged 4 commits intomainfrom
marko/attempt_rust_fix
Aug 4, 2025
Merged

fix: proto inclusion#2515
tac0turtle merged 4 commits intomainfrom
marko/attempt_rust_fix

Conversation

@tac0turtle
Copy link
Contributor

Overview

Due to how we have the repo setup, there is an issue about including the proto files in the crate, this is a solution to fix this

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

I've implemented a fix to address an issue with protobuf file inclusion within the crate. The primary goal of this change is to ensure that the crate can be built reliably, whether from source with .proto files present or from a published version where only pre-generated Rust files are available. This involves updating the crate's name in documentation and enhancing the build script to intelligently manage the generation and inclusion of protobuf-derived Rust code, making the build process more resilient and user-friendly.

Highlights

  • Crate Renaming and Documentation Update: I've updated the README.md file to reflect the new crate name ev-types (formerly rollkit-types) and adjusted all related documentation, including dependency examples and descriptions, to align with Ev-node.
  • Optimized Protobuf Generation Logic: I've modified the build.rs script to intelligently handle protobuf file generation. It now first checks for pre-generated .rs files in src/proto. If they exist and the EV_TYPES_FORCE_PROTO_GEN environment variable is not set, it will use these existing files, avoiding unnecessary regeneration.
  • Improved Proto File Inclusion Robustness: The build.rs script now includes robust error handling for the proto_root path. If the original .proto source files are not found (e.g., when building from a published crate where only generated files are needed), but the pre-generated .rs files are present, the build will proceed successfully, ensuring better portability and build reliability.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2025

The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedAug 4, 2025, 2:11 PM

@tac0turtle tac0turtle changed the title fix: fix proto inclusion fix: proto inclusion Aug 4, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the protobuf generation process to use pre-generated files by default, which is a good improvement for downstream users of the crate. It also adds an environment variable to force regeneration for developers. The documentation in README.md has been updated to reflect these changes. My review identified a logic flaw in the build script where the force-regeneration flag is ignored in an error-handling path. I've provided a specific suggestion to fix this to ensure the build script behaves as expected.

@claude
Copy link
Contributor

claude bot commented Aug 4, 2025

Claude finished @tac0turtle's task —— View job


Code Review for Proto Inclusion Fix

I've analyzed the changes and here's my comprehensive review:

Todo List:

  • Read and analyze the changed files
  • Review the build script logic and proto inclusion approach
  • Check for potential bugs and issues
  • Evaluate performance and security considerations
  • Assess test coverage needs
  • Provide comprehensive feedback

Overview

This PR implements a robust solution for proto file inclusion that allows the crate to work both from source (with .proto files) and when published to crates.io (with pre-generated files). The approach is sound and addresses a real packaging problem.

Code Quality & Best Practices ✅

Strengths:

  • Well-documented approach in README.md with clear usage instructions
  • Proper separation of concerns between message-only and full gRPC builds
  • Good error handling with informative messages
  • Clear feature flag design (grpc, transport)

Areas for improvement:

  • Consider adding more detailed inline comments in build.rs for complex logic sections

Critical Bug 🚨

Issue in build.rs:28-38 - Logic flaw with force regeneration flag

The Gemini Code Assist review correctly identified a significant logic bug. When EV_TYPES_FORCE_PROTO_GEN=1 is set but the proto source directory doesn't exist, the build script will fall back to using pre-generated files instead of failing as expected.

Current problematic flow:

  1. User sets EV_TYPES_FORCE_PROTO_GEN=1 to force regeneration
  2. canonicalize() fails because ../../../proto doesn't exist
  3. Code checks if pre-generated files exist and uses them
  4. User's explicit request to regenerate is silently ignored

Recommended fix:

Err(e) => {
    if force_regen {
        return Err(format!("EV_TYPES_FORCE_PROTO_GEN is set, but proto source files not found at ../../../proto: {}", e).into());
    }
    // If proto files don't exist but generated files do, that's ok
    if messages_file.exists() && services_file.exists() {
        println!("cargo:warning=Proto source files not found at ../../../proto, using pre-generated files");
        return Ok(());
    }
    // Otherwise, this is a real error
    return Err(format!("Proto files not found and no pre-generated files available: {}", e).into());
}

Additional Issues

1. Potential File Collision in build.rs:60-64, 74-78

Both prost_build and tonic_build generate files named evnode.v1.rs. The current approach renames them sequentially, but this could cause issues if the second generation overwrites the first before renaming.

Safer approach:

// Use different output directories or filenames initially
prost_config.out_dir(&proto_dir.join("messages"));
// ... then move to final location

2. Missing Error Handling for File Renames

The fs::rename() calls in lines 63 and 77 could fail, but the code continues without proper error propagation.

Performance Considerations ✅

  • Good: Pre-generated files avoid runtime compilation overhead
  • Good: Early exit when files exist and no force regen is requested (build.rs:20-23)
  • Good: Efficient file walking with walkdir and filter_map pattern

Security Considerations ✅

  • Good: No unsafe code or external process execution
  • Good: Proper path handling with canonicalize() to resolve symlinks/traversal attempts
  • Good: No hardcoded secrets or sensitive data exposure
  • Minor: Consider validating proto file extensions more strictly

Test Coverage Assessment ⚠️

Missing test scenarios:

  • Unit tests for the build script logic
  • Integration tests for different build scenarios:
    • Building with pre-generated files only
    • Building with proto sources
    • Force regeneration behavior
    • Error conditions (missing files, permission issues)

Recommended test structure:

#[cfg(test)]
mod tests {
    // Test force_regen flag behavior
    // Test fallback to pre-generated files
    // Test error conditions
    // Test file rename operations
}

Documentation Quality ✅

The README.md updates are excellent:

  • Clear usage examples for different scenarios
  • Good explanation of the two-file approach (messages vs services)
  • Helpful troubleshooting section for protoc version differences
  • Proper documentation of environment variables

Overall Assessment

This is a well-thought-out solution to a real problem, but it has one critical bug that needs immediate attention. The force regeneration logic flaw could confuse developers and lead to unexpected behavior.

Priority:

  1. HIGH: Fix the force regeneration logic bug
  2. MEDIUM: Add proper error handling for file operations
  3. LOW: Consider adding build script tests
  4. LOW: Improve file collision handling

The approach of pre-generating and checking in proto files is sound for a published crate, and the feature flag design is clean and user-friendly.


@codecov
Copy link

codecov bot commented Aug 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@04637e6). Learn more about missing BASE report.
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2515   +/-   ##
=======================================
  Coverage        ?   73.61%           
=======================================
  Files           ?       72           
  Lines           ?     6716           
  Branches        ?        0           
=======================================
  Hits            ?     4944           
  Misses          ?     1371           
  Partials        ?      401           
Flag Coverage Δ
combined 73.61% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tac0turtle
Copy link
Contributor Author

tested and this works

@tac0turtle tac0turtle force-pushed the marko/attempt_rust_fix branch from 38dcdb0 to 9caf9e6 Compare August 4, 2025 14:10
@tac0turtle tac0turtle marked this pull request as ready for review August 4, 2025 14:10
Copy link
Member

@Manav-Aggarwal Manav-Aggarwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tac0turtle tac0turtle added this pull request to the merge queue Aug 4, 2025
Merged via the queue into main with commit adf60e0 Aug 4, 2025
35 of 37 checks passed
@tac0turtle tac0turtle deleted the marko/attempt_rust_fix branch August 4, 2025 15:29
@github-project-automation github-project-automation bot moved this to Done in Evolve Aug 4, 2025
alpe added a commit that referenced this pull request Aug 7, 2025
* main:
  fix: docs deployment (#2528)
  chore: merge docs repo into this repo (#2526)
  optimize: implement head index for BatchQueue.Next() to avoid O(n) slice re-slicing (#2519)
  build(deps): Bump github.com/rs/zerolog from 1.33.0 to 1.34.0 (#2525)
  chore: migrate from ipfs/go-log to zerolog across codebase (#2516)
  fix: proto inclusion (#2515)
  build(deps): Bump github.com/docker/docker from 28.2.2+incompatible to 28.3.3+incompatible in /test/docker-e2e in the go_modules group across 1 directory (#2503)
  refactor: replace Split in loops with more efficient SplitSeq (#2502)
  feat: expose ev-node P2P and RPC ports in docker compose (#2513)
  feat: create grpc execution environemnt (#2490)
@tac0turtle tac0turtle removed this from Evolve Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants