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

protoc not resolving arguments using include paths #353

Closed
dt opened this issue Jun 29, 2021 · 5 comments
Closed

protoc not resolving arguments using include paths #353

dt opened this issue Jun 29, 2021 · 5 comments
Labels
Bug Something isn't working

Comments

@dt
Copy link

dt commented Jun 29, 2021

Given some directory layout under the current working directory like pkg/a/a.proto and lib/b/b.proto, this protobuf protoc invocation succeeds: protoc --go_out=tmp -I pkg -I lib a/a.proto. Note that the argument "a/a.proto" is not a path to a file in current working directory and is relying on being resolved using -I pkg.

The corresponding buf protoc invocation looks like it is not using the -I pkg to resolve its arguments:
buf protoc --go_out=tmp -I pkg -I lib a/a.proto returns

path "/Users/david/code/protodemo/a/a.proto" is not contained within any of roots "/Users/david/code/protodemo/lib", and "/Users/david/code/protodemo/pkg" - note that specified paths cannot be roots, but must be contained within roots

@bufdev
Copy link
Member

bufdev commented Jun 29, 2021

I can reproduce.

A note - generally protoc works with actual relative paths, i.e. pkg/a/a.proto, but yes it appears that it works with either relative or "include relative" paths. Which leads to some interesting questions that will need answers.

We'll do some investigation, thanks.

@bufdev bufdev added the Bug Something isn't working label Jun 29, 2021
@bufdev
Copy link
Member

bufdev commented Jun 29, 2021

This is a more complicated problem than it first seems - note that protoc works with either the actual path, or the include-relative path, which leads to situations like:

# what if both $(pwd)/pkg/a/a.proto and $(pwd)/a/a.proto exist?
protoc -I pkg -I . pkg/a/a.proto a/a.proto

Like most things involving mainline protoc, it's extremely error-prone. buf avoids all of this outside of buf protoc by having stricter include directory requirements that everyone should follow https://docs.buf.build/build-configuration#root-requirements but the goal of buf protoc is to be directly compatible, so tldr we have a lot of rule-relaxing to do, and it's not trivial to do safely.

We're working on it.

@dt
Copy link
Author

dt commented Jun 30, 2021

I wonder if anyone actually relies on duplicate / ambiguous cases like that. Maybe just me, but I'd almost prefer an error if the same input filename could be resolved to two different files?

@bufdev
Copy link
Member

bufdev commented Jun 30, 2021

I'd prefer an error for all of this tbh, hence why buf errors if you do any of these things, but we should match things up with protoc.

The reason why we outlaw i.e. -I . -I pkg is actually the duplicate case - protoc detects i.e. pkg/a/a.proto and a/a.proto as different files, but then will fail if you import them in separate ways (which -I . -I pkg allows). Example:

$ tree
.
├── lib
│   ├── b
│   │   └── b.proto
│   └── c
│       └── c.proto
└── pkg
    └── a
        └── a.proto
// pkg/a/a.proto
syntax = "proto3";

package a;

message Foo {}

// lib/b/b.proto
syntax = "proto3";

package b;

// import based on -I .
import "pkg/a/a.proto";

message Bar {
  a.Foo foo = 1;
}

// lib/c/c.proto
syntax = "proto3";

package c;

// import based on -I pkg
import "a/a.proto";

message Bar {
  a.Foo foo = 1;
}

Then:

$ protoc -I . -I pkg -o /dev/null pkg/a/a.proto lib/b/b.proto lib/c/c.proto
a/a.proto:5:9: "a.Foo" is already defined in file "pkg/a/a.proto".
lib/c/c.proto:5:1: Import "a/a.proto" was not found or had errors.
lib/c/c.proto:8:3: "a.Foo" seems to be defined in "pkg/a/a.proto", which is not imported by "lib/c/c.proto".  To use it here, please add the necessary import.

This happens a ton, which is why we enforce https://docs.buf.build/build-configuration#root-requirements

@bufdev
Copy link
Member

bufdev commented Feb 4, 2022

We have now deleted buf protoc by moving it to alpha in #915, and will likely eventually delete it entirely. As such, going to close this issue as buf protoc will almost for sure not be worked on in the future. If we decide to revisit this decision, we should reopen this issue. All issues that were closed are tracked on the #915.

@bufdev bufdev closed this as completed Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants