search

helsing-ai / buffrs

Modern protobuf package management
https://crates.io/crates/buffrs
Apache License 2.0
214 stars 12 forks source link

Vendor folder structure seems to be changing or broken #196

Closed heatonmatthew closed 10 months ago

heatonmatthew commented 10 months ago

I love what you're doing on this project. It's almost exactly what I was thinking of building too.

Problem

I'm trying to use the dependency capability of buffrs but can't understand the design direction of the /proto/vendor folder and the structure of the package .tgz file.

As an example, if I have a lib package with the following structure:

.
├── Proto.toml
└── proto
    └── type
        └── date.proto
    └── vendor

and Proto.toml file of:

[package]
type = "lib"
name = "google"
version = "0.1.0"

[dependencies]

Version 0.7.1

In version 0.7.1, when I run buffrs install, nothing is put in the vendor folder and when I run buffrs package the .tgz file contains the contents of the proto folder at the root of the tarball.

When I run buffrs lint it seems to use ./proto as the import path.

Version 0.7.2

In version 0.7.2, when I run buffrs install, it puts the files from the proto folder into the vendor folder and when I run buffrs package the .tgz file contains the contents of the proto folder underneath a path of /vendor/google/<proto-content-here>.

When I run buffrs lint it seems to use ./proto/vendor as the import path.

Questions

I have the following questions:

Thanks.

mara-schulke commented 10 months ago

Hi @heatonmatthew! Thank you for your question 😊

As a disclaimer upfront: We buffrs is in the active development phase and converging on design choices slowly. This means that we are introducing potentially breaking changes for quite some time to make sure that we are able to cover all common use cases that were not known when initially designing.

The change of "installing" the current package in the vendor directory introduced in 0.7.2 is related to #167 / #188 which enable people to do imports from your current package.

Background for this change

Imagine a slightly adapted version of your example:

.
├── Proto.toml
└── proto
    ├── datetime.proto
    ├── type
    |   ├── time.proto
    |   └── date.proto
    └── vendor

And you now in your datetime.proto you want to import both message Time and message Date to compose them:

syntax = "proto3";

// imports

package your_pkg;

message DateTime {
    your_pkg.type.Date date = 1;
    your_pkg.type.Time time = 2;
}

Until Buffrs 0.7.2 this was not possible as you couldn't do relative (e.g. import "./type/time.proto) in protobuf when the protoc include path is not set to the root path. So there was no valid import statement that you could use for this scenario.

This is why we opted for / desgined an import system that requires you to do absolute imports at all times (e.g. import "your_pkg/type/date.proto";) at all times. Buffrs ensures that this import is valid and picked up by your code completion tool through installing the local package before any operation (linting, installing, publishing, etc.) into the vendor directory so that the import path is actually valid when the protoc include path is set to proto/vendor;

Your Questions

I presume that the structure is changing to place all content under the /proto/vendor folder under package names when doing buffrs install (which is different to the examples in the documentation current)?

Yes exactly. buffrs install places all packages (now including the local) under proto/vendor so that you can compile the vendor folder only! Prior to this version people compiled the proto folder directly, which is not possible with a self-referencing enabled import system sadly. Thus users are now required to compile the proto/vendor folder after installing: E.g. protoc --cpp_out ./cpp --include proto/vendor $(buffrs ls)

Could you link the place which confused you?

Is this expected to be reflected in within the .tgz? No! The .tgz should only contain the top-level protocol buffers, if this is not the case you found a bug! I'll verify this asap. Sorry for the inconvenience.

When importing this package from another via a [dependencies] entry, under 0.7.2 it seems to do a double nesting of the package within the vendor folder (i.e. it unpackages the aforementioned google package to /proto/vendor/google/vendor/google). Presumably, this is a bug, possibly because the .tgz isn't expected to have the nested /vendor/ in it?

Yes this is unintended behavior and should have been catched by our testsuite. Sorry!

Is there any way I can assist in this?

Yes! If you want to contribute, I am happy to assist at all times. The bug here lies in the PackageStore::release function which is packaging the wrong protocol buffers. It should always package proto/**/*.proto excluding proto/vendor/**/*.proto but it seems to do the inverse.

If you want to pick this up – please let me know! – otherwise I will schedule someone to work on this (or pick it up myself).

Have a nice day and thank your for identifying this! ☺️

mara-schulke commented 10 months ago

This is repaired in v0.7.3 😊

heatonmatthew commented 10 months ago

Excellent. Thank you @mara-schulke this is what I'd imagined might be the case. :)

I understand that this is under active development and am happy with that. Given that it has been resolved in v0.7.3, the documentation matches what I'd expect (i.e. it was only incorrect relative to the buggy behaviour I found in v0.7.2).

I appreciate the response.

qsantos commented 10 months ago

For reference, the behavior was introduced in v0.7.2 by 66e321e727376eb5a09ab03a4d7fec1ae1f164e8 and has been fixed in main by 5dad8eb1960da9eafc23455feda9617aa15d6273, which is v0.7.3.

Edit: the tag v0.7.3 was recreated and I had an older version which was pointing to 5d17f01 instead of 5dad8eb1960da9eafc23455feda9617aa15d6273

mara-schulke commented 10 months ago

@heatonmatthew you may be interested in #201 to prevent issues like this in the future!

heatonmatthew commented 10 months ago

Thanks @mara-schulke