Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(build): Add option to emit rerun-if-changed instructions #1021

Conversation

mickvangelderen
Copy link
Contributor

Fixes: #1020
Refs: #1019

Motivation

Currently, tonic-build does not emit cargo:rerun-if-changed
instructions. This will cause the build script to be rerun if any file
changes in the package.

Solution

This PR implements an option on the Builder called
emit_rerun_if_changed that, when enabled, will emit the
cargo:rerun-if-changed instructions for all of the proto file paths
and include directories.

Currently, tonic-build does not emit cargo:rerun-if-changed
instructions. This will cause the build script to be rerun if any file
changes in the package.

This commit implements an option on the Builder called
emit_rerun_if_changed that, when enabled, will emit the
cargo:rerun-if-changed instructions for all of the proto file paths
and include directories.

Fixes: hyperium#1020
Refs: hyperium#1019
@mickvangelderen mickvangelderen marked this pull request as ready for review June 17, 2022 21:05
Copy link
Contributor Author

@mickvangelderen mickvangelderen left a comment

Choose a reason for hiding this comment

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

Do I need to edit a changelog or any of the examples?

@@ -27,6 +27,7 @@ pub fn configure() -> Builder {
emit_package: true,
protoc_args: Vec::new(),
include_file: None,
emit_rerun_if_changed: std::env::var_os("CARGO").is_some(),
Copy link
Contributor Author

@mickvangelderen mickvangelderen Jun 17, 2022

Choose a reason for hiding this comment

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

Not sure if run-time detection of whether the code is run as a build script is

  1. right, and
  2. necessary

Setting it to true may constitute a breaking change if people are using tonic_build outside of build scripts.
We could also set it to false in this version of tonic_build and make it true or automatically detected in the next major release.

Copy link
Member

Choose a reason for hiding this comment

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

I think its fine to set this for a build script. Do you know what other projects with similar features use? I don't mind auto enabling this in this release since I don't really think its a breaking style change etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I skimmed through the cargo reference but did not find anything else. I also failed to find results for how to detect Rust code is being run from a build script through Google. I hope this does a good enough job.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it seems fine to me.

Comment on lines 373 to 375
/// Enable or disable emitting the rerun-if-changed cargo instruction
///
/// If set, emits rerun-if-changed instructions so that Cargo understands when to rerun the build script.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the only place I need to document this option?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this is fine, I would also add info about the default setting that if you're in a build script this will be enabled and outside of a build script it won't

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Overall looks good!

Comment on lines 373 to 375
/// Enable or disable emitting the rerun-if-changed cargo instruction
///
/// If set, emits rerun-if-changed instructions so that Cargo understands when to rerun the build script.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this is fine, I would also add info about the default setting that if you're in a build script this will be enabled and outside of a build script it won't

@@ -27,6 +27,7 @@ pub fn configure() -> Builder {
emit_package: true,
protoc_args: Vec::new(),
include_file: None,
emit_rerun_if_changed: std::env::var_os("CARGO").is_some(),
Copy link
Member

Choose a reason for hiding this comment

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

I think its fine to set this for a build script. Do you know what other projects with similar features use? I don't mind auto enabling this in this release since I don't really think its a breaking style change etc.

@LucioFranco
Copy link
Member

@mickvangelderen just need you to run a fmt pass

@mickvangelderen
Copy link
Contributor Author

@mickvangelderen just need you to run a fmt pass

That was sloppy, sorry.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -27,6 +27,7 @@ pub fn configure() -> Builder {
emit_package: true,
protoc_args: Vec::new(),
include_file: None,
emit_rerun_if_changed: std::env::var_os("CARGO").is_some(),
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it seems fine to me.

@LucioFranco LucioFranco changed the title tonic-build: Add option to emit cargo:rerun-if-changed instructions feat(build): Add option to emit rerun-if-changed instructions Jun 21, 2022
@LucioFranco LucioFranco merged commit 1d2083a into hyperium:master Jun 21, 2022
@LucioFranco
Copy link
Member

and no problem! its easy to miss the formatting, I usually just have it auto fmt on save.

@mickvangelderen mickvangelderen deleted the emit-rerun-if-changed-from-tonic-build branch June 21, 2022 18:09
@mickvangelderen
Copy link
Contributor Author

@LucioFranco thanks for the help getting this in!

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.

Automatically emitting rerun-if-changed instructions for cargo
2 participants