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

Place ttrpc files in OUT_DIR #483

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Conversation

jprendes
Copy link
Collaborator

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.

Ensure protoc and dev tools have been installed. Run `make build` or to just generate the protos:

```
cargo build -p containerd-shim-wasm --no-default-features --features generate_bindings
Copy link
Member

Choose a reason for hiding this comment

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

it probably will make it difficult to discover where the generated files are. Could you please add some guidence on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The actual location of OUT_DIR is an implementation detail of cargo.

Do they need to be discoverable?
The .proto file is discoverable and what we should care about.

As I see it, the generated files are just an artifact.
They are not really intended for human consumption, we don't even lint them.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jsturtevant
Copy link
Contributor

LGTM

Mossaka
Mossaka previously approved these changes Feb 15, 2024
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

lgtm!

@Mossaka
Copy link
Member

Mossaka commented Feb 15, 2024

It seems like this needs a rebase.

@cpuguy83
Copy link
Member

Why would we not commit the files to the repo?

@jprendes
Copy link
Collaborator Author

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.

Signed-off-by: Jorge Prendes <[email protected]>
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

LGTM!

@cpuguy83
Copy link
Member

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.

@cpuguy83 cpuguy83 merged commit 7d436d9 into containerd:main Feb 23, 2024
43 checks passed
@jprendes
Copy link
Collaborator Author

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

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.

4 participants