search

containerd / runwasi

Facilitates running Wasm / WASI workloads managed by containerd
Apache License 2.0
1.06k stars 88 forks source link

Place `ttrpc` files in `OUT_DIR` #483

Closed jprendes closed 7 months ago

jprendes commented 7 months ago

This PR places the generated ttrpc files in OUT_DIR, and makes the files generation unconditional. This avoid commiting the generated files to the repo.

jsturtevant commented 7 months ago

LGTM

Mossaka commented 7 months ago

It seems like this needs a rebase.

cpuguy83 commented 7 months ago

Why would we not commit the files to the repo?

jprendes commented 7 months ago

Why would we not commit the files to the repo?

It's generally good practice not including generated files.

They are a distraction and can be confusing. It adds noise to the makefile, which is currently in charge of generating these files before building. It adds a seemingly useless (for downstream consumers) feature to the crate. Also contributors might think it's ok to modify them manually, just for their changes to be lost the next time they are generated. Their content is an implementation detail of ttrpc-rust, not particularly human friendly code. Also, if we move to shims v3, are we going to have the ttrcp-rust generated files for ttrcp and the tonic generated files for the grpc implementation?

There's also no benefit to having them in the repo. Build time impact is negligible (they are generated only once, cargo keeps track of this). It does need protoc installed, but we already need it for rust-extensions.

cpuguy83 commented 7 months ago

This is fine I guess. I expect it to break code lookups since those types are no longer there, but I guess the files are being generated when it compiles them for code analysis.

jprendes commented 7 months ago

Lookup works fine with rust-analyzer on vscode. As @jsturtevant mentioned, it's the same approach as in containerd/rust-extensions