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

protobuf gen triggers less often #7565

Merged

Conversation

Projects
None yet
5 participants
@illicitonion
Copy link
Contributor

commented Apr 15, 2019

Currently it triggers every rust build, which slows things down a lot.

Instead, only trigger if generated code actually changes.

@illicitonion illicitonion requested a review from blorente Apr 15, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

I can’t comment to the actual code, but yay! Thanks.

@jsirois
Copy link
Member

left a comment

Yay!

@stuhood
Copy link
Member

left a comment

Thanks!

}
// 1026

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 16, 2019

Member

Is this trailing comment necessary?

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 16, 2019

Author Contributor

Oops, leftover from when I was testing when things trigger :)

protobuf gen triggers less often
Currently it triggers every rust build, which slows things down a lot.

Instead, only trigger if generated code actually changes.
@blorente
Copy link
Contributor

left a comment

I have learnt many things on this PR, looks good thanks!

let mut rustfmt_config = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
rustfmt_config.pop(); // bazel_protos
rustfmt_config.pop(); // process_execution
rustfmt_config.push("rustfmt.toml");

This comment has been minimized.

Copy link
@blorente

blorente Apr 16, 2019

Contributor

It's unfortunate that we don't have a good way of not depending on this dir structure, something like CARGO_WORKSPACE_DIR. However, would it make sense for this to fail with a nice error here? Something like panic!("I assumed that rustfmt.toml was two levels up from {}, but the directory structure seems to have changed", env!("CARGO_MANIFEST_DIR")).

Maybe not worth the effort since it will error anyway, I just don't like having to figure out the implicit dependency when it changes.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 16, 2019

Author Contributor

Done

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/protogen/idempotent branch from c2a499e to a7c295e Apr 16, 2019

@illicitonion illicitonion merged commit c77bb4c into pantsbuild:master Apr 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/protogen/idempotent branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.