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

Lacking support of `protoc` equivalent `--proto_path` option #39

Closed
zhouyan opened this Issue Jan 7, 2018 · 14 comments

Comments

Projects
None yet
5 participants
@zhouyan

zhouyan commented Jan 7, 2018

Maybe I missed it, but there seems to be no support for something similar to --proto_path or -I option as found for other languages's protoc based implementation. For example,

import "google/protobuf/timestamp.proto";

will not work out of box. And this is common enough usage of ProtoBuf.

@murraystokely

This comment has been minimized.

Collaborator

murraystokely commented Jan 7, 2018

@zhouyan

This comment has been minimized.

zhouyan commented Jan 7, 2018

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Jan 7, 2018

Well

  • Maybe collate them by hand
  • Work out a PR to add an include mechanism

It so happens that I was musing about an #include equivalent for reading TOML files (in C++) and my pal @dcdillon actually developed a fully generic solution for input streams (which we still need to showcase better). That could work here too, but someone needs to do the work, and you are the one with the itch.

Unless, of course, @murraystokely comes up with another google-y trick already present in the library. I half expect it to be there too...

@zhouyan

This comment has been minimized.

zhouyan commented Jan 7, 2018

For example,

import "google/protobuf/timestamp.proto";

message Foo {
    google.protobuf.Timestamp ts = 1;
}
readProtoFiles("foot.proto")

will always lead to error google/protobuf/timestamp.proto:0:1:File not found.

A workaround I find is that to put all dependencies in one directory, for example, say the current working directory is as the following,

  • google/protobuf/timestamp.proto
  • foo.proto

Then it works. But now consider that foot.proto is to be distributed with package Foo, And there's another bar.proto to be distributed with package Bar. If only one of them is used, then we can put google/protobuf along with the package protofiles and change working directory temporarily in some package initialization steps. However, because now timestamp.proto appears in two places, and try to read it twice from two places will leads to error like "google.protobuf.Timestamp" is already defined in file "google/protobuf/timestamp.proto".

What's worse, say now package Foo depends on package Bar and want to import bar.proto, it just get even messier.

In short, the current implementation only really work for self-contained proto files, which isn't really the most commonly usage of ProtoBuf in general

@murraystokely

This comment has been minimized.

Collaborator

murraystokely commented Jan 7, 2018

@siddharthab

This comment has been minimized.

Contributor

siddharthab commented Jan 18, 2018

The --proto_path option that you mention is passed on as getwd() and the directory of the included files by default. There is currently no intuitive mechanism for providing a custom directory, other than the indirect way (by making your current dir the same as the include path and using dir instead of files) mentioned by @murraystokely above which is difficult to discover, and will not work when there are two include directories that have imports into each other.

directories <- unique( c( getwd(), dirname(files) ) )

There is another strange behavior in which files argument are converted to their absolute paths. So if RProtoBuf reads them once as their absolute path, every import statement in a subsequently read proto file should also refer them with their absolute paths to match the originally read file.

I tried to make this behavior consistent with protoc, but it was difficult to do so without introducing breaking changes from existing behavior, especially where file descriptors (and import statements for them) are being keyed by absolute paths.

So I introduced a new function in #40.

@jcaberio

This comment has been minimized.

jcaberio commented Mar 22, 2018

hello.
i've manage to make a minimal package that demonstrates the need for --proto_path option.

https://github.com/jcaberio/pbimport/tree/master

@siddharthab

This comment has been minimized.

Contributor

siddharthab commented May 20, 2018

This is now resolved by #40.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented May 20, 2018

@siddharthab Is there an email address I can use for you in the ChangeLog ?

@siddharthab

This comment has been minimized.

Contributor

siddharthab commented May 20, 2018

bagaria.siddhartha@gmail.com

Thank you again!

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented May 20, 2018

Lovely, thanks. If you happen to have a unit test or two hanging around for this, or feel like cooking one up I would gladly take them to complete this further. NEWS and ChangeLog entry coming in a minute...

@siddharthab

This comment has been minimized.

Contributor

siddharthab commented May 20, 2018

I have these two commits -- master...siddharthab:unit

Description of these tests is in #40 (comment)

The tests will work but to make them proper, we should also be clearing persisted state between two unit tests. We will need a utility function in C++ called before/after each test.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented May 20, 2018

Right, I recalled some tests being part of the discussion.

Is it woth cooking up a quick helper function to purge state? Do you want to give it a shot?

@siddharthab

This comment has been minimized.

Contributor

siddharthab commented May 21, 2018

Done -- #45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment