Rust build cache invalidated by build.rs execution

[tiziano88]

I noticed that, even without changing any Rust code, if I re-run a build or tests, it always takes a long time for the command to finish, while I would normally expect most artifacts and test results to be cached.

I suspect the issue is that we use build.rs scripts in a number of places in our repository, mostly to generate protobuf objects, and somehow that invalidates the build cache. I am not sure it is an issue with build scripts in general, or the particular steps that our build scripts execute.

I note that, by removing (or renaming) all the build scripts from our repository, the build and test results seem indeed to be cached, and these commands run very quickly.

[tiziano88]

I have not tried leaving around the build.rs file, but with an empty main, I guess that could be a useful test to figure out if the issue is due to the presence of these files, or their contents. If it turns out to be caused by protoc_rust_grpc, we should then investigate how to mitigate it.

[daviddrysdale]

#514 possibly related?

[tiziano88]

To reproduce, run the following command twice, notice the second time it rebuilds all the crates that include a build.rs file, if present. Other crates are cached instead.

cargo build --release --target=wasm32-unknown-unknown --manifest-path=examples/hello_world/module/rust/Cargo.toml

[ipetr0v]

Why do we need grpc-compiler hardcoded into third_party ?
https://github.com/project-oak/oak/tree/2a2a3e4d491b1847b0e10390166252d62ce16424/third_party/grpc-rust/grpc-compiler

oak/examples/hello_world/module/rust/Cargo.toml

Lines 24 to 25 in 2a2a3e4

	[build-dependencies]
	protoc-rust-grpc = { path = "../../../../third_party/grpc-rust/protoc-rust-grpc" }

Can we download it from crates.io instead?

[tiziano88]

We maintain a forked version, the code generation is different.

[ipetr0v]

Could it be that grpc-compiler (and protoc-rust-grpc) doesn't use Cargo's cache?

[ipetr0v]

rust-protobuf always rewrites generated files and doesn't check if they have not changed:
https://github.com/stepancheg/rust-protobuf/blob/9d81264745951cb9491179be4f9a50a8bc1ce951/protoc-rust/src/lib.rs#L143-L149
https://github.com/stepancheg/rust-protobuf/blob/4612c86fd3eb06a800dcef098e1e238668c335cf/protobuf-codegen/src/lib.rs#L315-L326

So IIUC the problem is not caused by grpc-rust but by rust-protobuf.

[ipetr0v]

I'm running Rust tests and receive this error in random places:

error[E0425]: cannot find function `type_name` in module `any`
   --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-serialize-610.0.0/serialize.rs:852:21
    |
852 |                any::type_name::<S>(),
    |                     ^^^^^^^^^ not found in `any`
help: possible candidates are found in other modules, you can import them into scope
    |
7   | use core::intrinsics::type_name;
    |
7   | use std::intrinsics::type_name;
    |

error[E0425]: cannot find function `type_name` in module `any`
   --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-serialize-610.0.0/serialize.rs:854:21
    |
854 |                any::type_name::<T>(),
    |                     ^^^^^^^^^ not found in `any`
help: possible candidates are found in other modules, you can import them into scope
    |
7   | use core::intrinsics::type_name;
    |
7   | use std::intrinsics::type_name;
    |

[ipetr0v]

Since I have added protoc-rust-grpc to dependencies in oak_utils, I started to receive the following errors:

error: struct update has no effect, all the fields in the struct have already been specified
  --> /opt/my-project/third_party/grpc-rust/protoc-rust-grpc/src/lib.rs:44:15
   |
44 |             ..Default::default()
   |               ^^^^^^^^^^^^^^^^^^
   |
   = note: `-D clippy::needless-update` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_update
error: Statics have by default a `'static` lifetime
  --> /opt/my-project/third_party/grpc-rust/protoc-rust-grpc/src/lib.rs:71:28
   |
71 |         static DOT_SLICE: &'static [&'static str] = &["."];
   |                           -^^^^^^^--------------- help: consider removing `'static`: `&[&'static str]`
   |
   = note: `-D clippy::redundant-static-lifetimes` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes

error: Statics have by default a `'static` lifetime
  --> /opt/my-project/third_party/grpc-rust/protoc-rust-grpc/src/lib.rs:71:38
   |
71 |         static DOT_SLICE: &'static [&'static str] = &["."];
   |                                     -^^^^^^^---- help: consider removing `'static`: `&str`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes

error: unneeded return statement
   --> /opt/my-project/third_party/grpc-rust/protoc-rust-grpc/src/lib.rs:137:9
    |
137 |         return Some(&path[prefix.len() + 1..]);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `Some(&path[prefix.len() + 1..])`
    |
    = note: `-D clippy::needless-return` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return

error: unneeded return statement
   --> /opt/my-project/third_party/grpc-rust/protoc-rust-grpc/src/lib.rs:139:9
    |
139 |         return None;
    |         ^^^^^^^^^^^^ help: remove `return`: `None`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return

Is it ok to change protoc-rust-grpc in the third_party, or it supposed to be an unchanged fork?

[tiziano88]

I realised that perhaps the root cause is that the build script emits files outside of OUT_DIR, against guidelines.

The build script is re-run when any change in the source files within the package is detected, based on timestamps, but generating proto files under src means that the compiler thinks the package is always modified.

@ipetr0v could you try using OUT_DIR and see if that works for us? I think that would mean that we would not be checking in the generated files, but perhaps that's what we should be doing anyways.

[ipetr0v]

Tried to use an example from rust-bindgen

pub mod oak_api {
    include!(concat!(env!("OUT_DIR"), "/oak_api.rs"));
}

It leads to the following errors:

error: an inner attribute is not permitted following an outer attribute
  --> /usr/local/google/home/ivanpetrov/src/oak/target/debug/build/oak_abi-70604563f2c30de3/out/oak_api.rs:18:3
   |
18 | #![allow(unused_imports)]
   |   ^
   |
   = note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files. Outer attributes, like `#[test]`, annotate the item follo
wing them.

error: an inner attribute is not permitted following an outer attribute
  --> /usr/local/google/home/ivanpetrov/src/oak/target/debug/build/oak_abi-70604563f2c30de3/out/oak_api.rs:19:3
   |
19 | #![allow(unused_results)]
   |   ^
   |
   = note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files. Outer attributes, like `#[test]`, annotate the item follo
wing them.

error: expected outer doc comment
  --> /usr/local/google/home/ivanpetrov/src/oak/target/debug/build/oak_abi-70604563f2c30de3/out/oak_api.rs:20:1
   |
20 | //! Generated file from `oak_api.proto`
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: inner doc comments like this (starting with `//!` or `/*!`) can only appear before items

So it's not possible to have inner attributes in the included files.

The workaround that I have found is to open the generated file, add 'mod proto { }' around the contents and write it back:
https://github.com/googlecartographer/point_cloud_viewer/blob/bb73289523a3cee8091e9b6547b7b989d0fc61c7/build.rs#L16-L33

[tiziano88]

@ipetr0v thanks for looking into it, I don't think it's worth exploring this more, let's stick to the current version, unless you think otherwise?

[tiziano88]

I think could be closed now @ipetr0v ?

[ipetr0v]

Yes, I agree