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

transitive buffrs resolution corrupts input crates #218

Open
j-baker opened this issue Jan 26, 2024 · 6 comments
Open

transitive buffrs resolution corrupts input crates #218

j-baker opened this issue Jan 26, 2024 · 6 comments
Assignees
Labels
bug Reports or fixes associated with bugs in this project complexity::medium Issues or ideas which require some discussion, research and more implementation work component::cli Everything related to the buffrs cli docs::book Adding, rewriting or removing chapters from the buffrs book docs::project Documenting the project vision, design decisions etc integration Changes and ideas related to integrating buffrs with 3rd-party software or tools priority::high Urgent change or idea, please review quickly
Milestone

Comments

@j-baker
Copy link
Collaborator

j-baker commented Jan 26, 2024

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 j-baker changed the title transitive buffrs resolution corrupts crates transitive buffrs resolution corrupts input crates Jan 26, 2024
@j-baker
Copy link
Collaborator Author

j-baker commented Jan 26, 2024

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

@mara-schulke
Copy link
Contributor

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 mara-schulke added bug Reports or fixes associated with bugs in this project component::cli Everything related to the buffrs cli priority::high Urgent change or idea, please review quickly integration Changes and ideas related to integrating buffrs with 3rd-party software or tools complexity::medium Issues or ideas which require some discussion, research and more implementation work docs::book Adding, rewriting or removing chapters from the buffrs book docs::project Documenting the project vision, design decisions etc labels Jan 29, 2024
@mara-schulke mara-schulke self-assigned this Jan 29, 2024
@mara-schulke mara-schulke added this to the Documentation milestone Jan 29, 2024
@asmello
Copy link
Collaborator

asmello commented Feb 15, 2024

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?

@mara-schulke
Copy link
Contributor

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
Copy link
Contributor

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

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

Should do the trick?

@bpoumarede
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Reports or fixes associated with bugs in this project complexity::medium Issues or ideas which require some discussion, research and more implementation work component::cli Everything related to the buffrs cli docs::book Adding, rewriting or removing chapters from the buffrs book docs::project Documenting the project vision, design decisions etc integration Changes and ideas related to integrating buffrs with 3rd-party software or tools priority::high Urgent change or idea, please review quickly
Projects
Status: No status
Development

No branches or pull requests

4 participants