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

Ruby generator: Use 'require_relative' instead of 'require'. #344

Closed

Conversation

atombender
Copy link

This fixes the (bad) assumption that the currently evaluated module's parent directory is in $LOADPATH and that require thus would be able to resolve dependencies.

Note that I'm not familiar enough with this project to know if file->dependency(n) actually always returns a relative path. If it does, this will not work, but I can amend the patch if that's necessary.

@cfallin
Copy link
Contributor

cfallin commented May 5, 2015

Hmm, I'm not sure that require_relative is actually correct as-is -- at least, we would need to fix up the path to make it relative.

file->dependency(n) returns the canonical name of the depended-on .proto file, which is relative to the protoc search root, not the location of file. So, for example, if a/b/c.proto imports a/b/d.proto, then we actually get the path a/b/d.proto when processing c.proto. This patch would try to load a file a/b/a/b/d.proto, which I don't think is what we want.

IMHO at least, requiring that the user set up their include/load paths to match their file hierarchy is not an eminently unreasonable thing (i.e., reasonable people could disagree, but it doesn't seem obviously wrong to me). FWIW, the C++ generator does a similar thing by translating imports to #include directives and relying on the user to set include paths.

Thoughts?

@atombender
Copy link
Author

I think it's non-idiomatic Ruby to assume that $LOAD_PATH contains the folder of the current file.

Ruby's require corresponds to #include <...>, whereas require_relative corresponds to #include "...". In my experience the former tends to be recommended for external dependencies (system libs and third-party libs), the latter for files internal to a project.

So even in C, willy-nilly includes of <service.h> would be a bad idea. What if your protobuf file is called math.proto, does it generate #include <math.h>?

In Ruby it's a common pattern to add a project's lib to the load path and organize files under lib/myapp. Your project then requires specific modules by using require "myapp/service" (often by having a common file lib/myapp.rb that packages all of these requires so you can do require "myapp" and get everything in one go). Including the root pretty much guarantees that the path does not conflict with, say, a gem or system-wide library called service.


Regarding locations, I found the behaviour of protoc strange and idiosynchratic. If I run (from a Ruby project root) protoc ../../go/src/github.com/myorg/myapp/service.proto, I end up with a file called lib/github.com/myorg/myapp/service.rb. I cannot imagine a situation where this would make sense. That path, after all, is a Go path. It seems wrong to impose a Go naming convention on a Ruby app, or any non-Go app. We certainly wouldn't want to pollute our nicely-organized projects this way. The build system would have to move the file afterwards, which seems unnecessary and tedious. And I haven't been able to find a flag to changing this file-name mapping.

@cfallin
Copy link
Contributor

cfallin commented May 5, 2015

OK, a few things:

  • I'm fine with using relative_require instead of require. To make this work (because of the a/b/a/b/d.proto problem I mentioned above), we would have to compute how to reach imported dependences relative to the current path, given those imported dependences' absolute paths. We're happy to take a PR for this.
  • Re: protoc's path-handling behavior in general: I'm afraid I'm not understanding the desired behavior. You say that github.com/myorg/... shouldn't be in the generated code's path (I think) -- but, it was in the .proto file's path. I'm not sure why you say that protoc "imposes a Go naming convention": it is just taking the path it's given and creating the same directory structure at the output. Perhaps let's approach from this direction: in an ideal world, what output filename should your example invocation produce? Then we can work backward from that and derive some principles that protoc can use to get that behavior :-)

So even in C, willy-nilly includes of <service.h> would be a bad idea. What if your protobuf file is called math.proto, does it generate #include <math.h>?

Not quite; in C++, math.proto becomes math.pb.h. The usual large-project layout we recommend for C++ is to set include paths at the root of the tree, invoke protoc from the root of the tree, and then a/b/c.proto becomes a/b/c.pb.h and the user can do #include "a/b/c.pb.h".

@cfallin
Copy link
Contributor

cfallin commented May 5, 2015

One suggestion that came to mind: perhaps we could add a flag that places all output files in the root of the output directory, regardless of input path? This matches the default behavior of, e.g., gcc -c. Or alternately, a new flag that specifies the exact output file name (and must be passed only when protoc is given a single .proto file on the input, not multiple ones). We can't change existing (default) behavior because many people rely on it, but perhaps one of these would satisfy your use case.

@pherl and/or @haberman: thoughts?

@acozzette
Copy link
Member

I'm going to close this because there's been no activity for a few years, but feel free to reopen it if this is still a problem.

@acozzette acozzette closed this Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants