tonic icon indicating copy to clipboard operation
tonic copied to clipboard

emit_rerun_if_changed inside workspace

Open cemoktra opened this issue 3 years ago • 5 comments

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

cemoktra avatar Aug 31 '22 11:08 cemoktra

Can you share a minimal example

eskawl avatar Dec 22 '22 12:12 eskawl

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

cemoktra avatar Dec 22 '22 13:12 cemoktra

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 avatar Sep 21 '23 08:09 kriswuollett

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);
    }

kriswuollett avatar Sep 21 '23 08:09 kriswuollett

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.

flokli avatar Mar 15 '24 21:03 flokli