hyperium / tonic

A native gRPC client & server implementation with async/await support.
https://docs.rs/tonic
MIT License
9.77k stars 997 forks source link

emit_rerun_if_changed inside workspace #1070

Open cemoktra opened 2 years ago

cemoktra commented 2 years ago

Bug Report

When using emit_rerun_if_changed in a workspace project it adds a rerun for the complete workspace folder. This causes way to many rebuilds.

Version

0.8

Description

Adding a rerun for the workspace folder causes rebuilding of all tonic-build crates even nothing relevant for those crates has changed

eskawl commented 1 year ago

Can you share a minimal example

cemoktra commented 1 year ago

I need to check if this still exists, not sure if i'm able to do this before xmas

kriswuollett commented 1 year ago

I believe it is a bug to emit rerun-if-changed for anything other than proto files. The set of proto files needed should be known during recursive processing and available in the dependency field of FileDescriptorProto:

  // Names of files imported by this file.
  repeated string dependency = 3;

I'm assuming the proto parent path may have been used as a substitute for calculating the actual file set. This is causing unnecessary rebuilds when using tonic in a monorepo and/or workspace.

I turned off any rust-analyzer that was watching the directory, and just ran cargo build multiple times with the same parameters. It always recompiles an "api" package of mine since the proto files are specified relative to an ancestor directory, i.e., the workspace root, which has target (which I assume is where files are changing after the "api" package is built). Here is the output of an example cargo build -vv:

       Dirty REDACTED-pkg-REDACTED-subpkg-REDACTED-api v0.1.0 (/Users/kris/Code/REDACTED/proj-REDACTED/REDACTED/pkg-REDACTED/subpkg-REDACTED/api): the file `REDACTED/pkg-REDACTED/subpkg-REDACTED/api/../../../..` has changed (1695282038.256505522s, 9s after last build at 1695282029.594488754s)
   Compiling REDACTED-pkg-REDACTED-subpkg-REDACTED-api v0.1.0 (/Users/kris/Code/REDACTED/proj-REDACTED/REDACTED/pkg-REDACTED/subpkg-REDACTED/api)
     Running `/Users/kris/Code/REDACTED/proj-REDACTED/target/debug/build/REDACTED-pkg-REDACTED-subpkg-REDACTED-api-8e24b3dc1b49640c/build-script-build`
[REDACTED-pkg-REDACTED-subpkg-REDACTED-api 0.1.0] cargo:rerun-if-changed=../../../../REDACTED/pkg-REDACTED/subpkg-REDACTED/v1/api.proto
[REDACTED-pkg-REDACTED-subpkg-REDACTED-api 0.1.0] cargo:rerun-if-changed=../../../../REDACTED/pkg-REDACTED/subpkg-REDACTED/v1/repo.proto
[REDACTED-pkg-REDACTED-subpkg-REDACTED-api 0.1.0] cargo:rerun-if-changed=../../../..

The build.rs:

use std::{collections::HashSet, env, path::PathBuf};

use prost::Message;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let proto_dir = "../../../..";
    let proto_files: HashSet<&str> = HashSet::from([
        "REDACTED/pkg-REDACTED/subpkg-REDACTED/v1/api.proto",
        "REDACTED/pkg-REDACTED/subpkg-REDACTED/v1/repo.proto",
    ]);
    let proto_file_paths: Vec<String> = proto_files
        .iter()
        .map(|&p| format!("{proto_dir}/{p}"))
        .collect();

    let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap());
    let descriptor_path = out_dir.join("descriptor.bin");

    let mut builder = tonic_build::configure()
        .protoc_arg("--experimental_allow_proto3_optional")
        .build_client(true)
        .file_descriptor_set_path(&descriptor_path)
        .out_dir(out_dir);

    #[cfg(feature = "with-tonic")]
    {
        builder = builder.build_server(true);
    }

    for t in vec!["Timestamp"] {
        builder = builder.extern_path(
            format!(".google.protobuf.{t}"),
            format!("::prost_types::{t}"),
        );
    }

    builder.compile(&proto_file_paths, &[proto_dir])?;

    // NOTE: Do not include dependencies in descriptor.
    // SEE: https://github.com/tokio-rs/prost/issues/880
    let descriptor_bytes = std::fs::read(&descriptor_path).unwrap();
    let mut descriptor = prost_types::FileDescriptorSet::decode(&descriptor_bytes[..]).unwrap();
    descriptor.file = descriptor
        .file
        .into_iter()
        .filter(|x| proto_files.contains(x.name()))
        .collect();
    std::fs::write(&descriptor_path, descriptor.encode_to_vec())?;

    Ok(())
}
kriswuollett commented 1 year ago

Workaround is easy assuming dependencies are already library dependencies, e.g., the well known types, but unfortunate this needs to be done:

    let mut builder = tonic_build::configure()
        .protoc_arg("--experimental_allow_proto3_optional")
        .build_client(true)
        // NOTE: Suppress rerun-if-changed for workspace directory
        // SEE: https://github.com/hyperium/tonic/issues/1070
        .emit_rerun_if_changed(false)
        .file_descriptor_set_path(&descriptor_path)
        .out_dir(out_dir);

    // NOTE: Add rerun-if-changed only listed files above 
    // SEE: https://github.com/hyperium/tonic/issues/1070
    for proto_file_path in proto_file_paths.iter() {
        println!("cargo:rerun-if-changed={}", proto_file_path);
    }
flokli commented 6 months ago

We had a similar problem with tvix, where a rerun-if-changed was emitted for the entire monorepo, due to the include path. It'd be really great if tonic-build could only emit rerun-if-changed for all .proto files it comes across.

For now we worked-around this problem by disabling emitting rerun-if-changed alltogether, but that of course comes with its own set of problems.