Skip to content

Only rebuild protos if proto source changes#2283

Merged
Geal merged 2 commits intoapollographql:devfrom
scottdouglas1989:fixes-cargo-build-cache-for-protos
Dec 21, 2022
Merged

Only rebuild protos if proto source changes#2283
Geal merged 2 commits intoapollographql:devfrom
scottdouglas1989:fixes-cargo-build-cache-for-protos

Conversation

@scottdouglas1989
Copy link
Contributor

tonic_build changes timestamps by a few nanos.

Router only needs to re-run tonic_build if reports.proto changes, and that's managed by the build.rs file already via println!("cargo:rerun-if-changed={}", reports_src.to_str().unwrap());

This just disables tonic_build's rerun emits so it's fully managed by apollo-router

@scottdouglas1989 scottdouglas1989 force-pushed the fixes-cargo-build-cache-for-protos branch from 60f3be3 to 69b51a4 Compare December 18, 2022 20:23
content = content.replace("[(js_preEncoded)=true]", "");
std::fs::write(&reports_out, &content)?;

println!("cargo:rerun-if-changed={}", reports_src.to_str().unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

why move this?

Copy link
Contributor Author

@scottdouglas1989 scottdouglas1989 Dec 20, 2022

Choose a reason for hiding this comment

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

When I was first investigating the rebuild, it was unclear to me with it further up, I ended up with it at the bottom of the build right before handing off to tonic_build.

Worth noting, the location of this line does not affect cargo's change detection.

@Geal Geal merged commit 0b1cc32 into apollographql:dev Dec 21, 2022
@Geal
Copy link
Contributor

Geal commented Dec 21, 2022

thanks for the help!

@abernix abernix added this to the v1.7.0 milestone Dec 22, 2022
@abernix abernix mentioned this pull request Dec 23, 2022
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.

5 participants