buffrs icon indicating copy to clipboard operation
buffrs copied to clipboard

transitive buffrs resolution corrupts input crates

Open j-baker opened this issue 1 year ago • 6 comments

Suppose I have:

FooApi - buffrs package, corresponds to buffrs.
FooClient - client for Foo, using this API. Let's say it uses the buffrs directly.

and then we have Bar which is in a different repo, depends on Foo from a registry.

What will happen when we run cargo build is approximately:

  1. FooClient will be downloaded into $CARGO_HOME/registry/cache/registry_id/foo-client-0.0.0.crate.
  2. It will be extracted into $CARGO_HOME/registry/cache/registry_id/foo-client-0.0.0.
  3. When the build happens, the crate's buildscript will have a target dir. This is where it supposed to write to. Cargo says:
OUT_DIR If the package has a build script, this is set to the folder where the build script should place its output. See below for more information. (Only set during compilation.)

OUT_DIR the folder in which all output and intermediate artifacts should be placed. This folder is inside the build directory for the package being built, and it is unique for the package in question.

Unfortunately, buffrs does not write its output here. Instead it writes down a few more protobuf files into $CARGO_HOME/registry/cache/registry_id/foo-client-0.0.0, because it's writing into some subdir of $CARGO_MANIFEST_DIR.

In my case, this is failing because my cargo manifest is in a read only directory.

I think that during Cargo builds, buffrs needs to be writing into $OUT_DIR in the cargo script, not into the source dir. Writing into the source dir makes sense during development, but not for published crates, because if buffrs somehow corrupted an input crate due to a bug (e.g. vendored a proto wrong), this would survive cargo cleaning.

For now I can work around by just not using the buffrs helpers in the cargo code (I have no dependencies) but this seems to be a misuse of Cargo.

j-baker avatar Jan 26 '24 19:01 j-baker

My sense is that vendored protobuf files will have to be checked into the repository as the real fix to all this stuff.

j-baker avatar Jan 26 '24 19:01 j-baker

For documentation:

We should probably solve theses issues by defining the include field in cargo manifests to zip up the protobufs in the crate tarball.

See https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields


A todo here would be to write a section in the buffrs book on how to publish crates to make this solution visible to people

mara-schulke avatar Jan 29 '24 12:01 mara-schulke

For documentation:

We should probably solve theses issues by defining the include field in cargo manifests to zip up the protobufs in the crate tarball.

See https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields

A todo here would be to write a section in the buffrs book on how to publish crates to make this solution visible to people

An annoying side-effect of this approach is that you have to then manually specify everything you want bundled in the crate's package. That's not very maintainable, even if you use globs. What is the rationale for not including the proto/vendor folder into version control?

asmello avatar Feb 15 '24 15:02 asmello

Well you can do that @asmello – it's just not very neat (in my personal opinion) to check in artifacts of package managers.

If if fits your needs better then just go ahead 😊

mara-schulke avatar Feb 15 '24 19:02 mara-schulke

Also fwiw @asmello if I understand the docs of cargo correctly:

include = ["*", "proto/vendor"]

Should do the trick?

mara-schulke avatar Feb 15 '24 19:02 mara-schulke

I tried both solution

include = [
  "Proto.toml",
  "Proto.lock",
  "proto/vendor",
  "proto/*.proto",
  "src",
  "build.rs",
]

or

checked into the repository

But that did not fix my issue.

bpoumarede avatar Feb 27 '24 14:02 bpoumarede