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

Add directory to support installing in a different directory than 'package-name' #236

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

poliorcetics
Copy link
Contributor

Closes #230

TODOs

  • Write tests for it: no test fails when I introduce these changes, I need to write tests where the directory key is something completely different to the package name
  • Migrations in registry: how to add them ? I need to add a column to the packages table.

@mara-schulke
Copy link
Contributor

Hi @poliorcetics, this looks like it will mess up the import system a lot to me; have you considered the implications of this change?

My main question here is what is the benefit of decoupling the protobuf import path with the buffrs package name? And if there is a benefit, isn't the scenario solvable by refactoring the underlying protos/adapting the build system?

@dgehriger
Copy link

Hi @mara-schulke. I have been looking into buffrs and it really looks very promising. However, I'm also suffering from the fact that the package statements don't allow dashes, while buffrs Proto.toml files require them.

As pointed out many times by others, this forces one to:

  • name a package in Proto.toml as e.g. name = api-examples-hub
  • name the same package in the .proto file as package api_examples_hub
  • import that package as import "api-examples-hub/foo.proto"
  • refer to its types as api_examples_hub.Type

From your comments, it seems that this is actually a feature in your setup. Maybe I misunderstand how you are using buffrs in your organization. My suspicion is that you aren't using hierarchical names. Is that the case? If not, why this restriction? Simply allowing . in the Proto.toml name would solve the problem, right?

@mara-schulke
Copy link
Contributor

Hi @dgehriger, yes I agree with the beauty of hierarchical package definitions! The problem with the proposed solution (as much as id like to implement it) is that it silently introduces ambiguity in which packages control which namespace in the generated code.

Ie. imagine that we have two packages org.foo and org.foo.bar - if the buffrs package name partially overlap and correspond directly to their protobuf package declaration this introduces unclarity on which buffrs package now owns defining org.foo.bar.Type because it could be defined in the org.foo package (as a subpackage) or in org.foo.bar toplevel.

So what I'm trying to point out is: If you introduce hierarchy into package names and allow the owned namespaces of buffrs packages to overlap, you can cause very nasty incompatibilities between packages that won't compile anymore: Imagine both of the example packages declaring the same org.foo.bar.Type – if you install both buffrs packages and try to compile them all it will brick.

The easy solution we went here (instead of doing rather complex analysis of package contents to verify they don't declare any subpackages or overlapping types) is to bind the buffrs package name to its very own protobuf package scope. This is pretty much inspired from rust fwiw.

I'm pretty sure that there is a design that does both? But we haven't had the time to implement this properly so far and there would be a unclear ROI of this feature as it is mostly cosmetics (for Helsing) - for companies trying to port over existing hierarchical apis this does look different. I'm happy for any PRs on this!

@mara-schulke
Copy link
Contributor

Edit: If you are only talking about the directory that buffrs installs its files into yes that is infact right. The reason we have not implemented this change is that there have been plans on enforcing the alignment of the protobuf package names with the buffrs one.

Currently this is only a lint (see buffrs lint) and not something that prevents you from publishing but I would very much advise against starting with this for the aforementioned reasons.

So it's best to align protobuf package name and buffrs package name for disambiguating types and preventing compilation errors - further allowing the buffrs package name to diverge from the protobuf package name in the current way things are implement would only worsen the problem.

So recapping the above:

  • I'm happy for this to change IF there are strict rules for the contents of a package
  • All considerations were taken to ensure this is not yielding a more painful user experience (ie. not being able to define subpackages can get pretty annoying in big projects)

@dgehriger
Copy link

dgehriger commented Jun 26, 2024

@mara-schulke Thank you very much for your detailed answer. I went ahead and implemented a possible solution (opt-in) in PR #251. Let me know what you think!

@dgehriger
Copy link

@mara-schulke : I totally get your point about the ambiguity arising from using nested packages. My ultimate goal is to be able to us buf for linting. I'll update this thread later today with my proposed repository layout to make that work.

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.

Buffrs enforces inconsistencies between package directive and import paths
3 participants