|
|
Created:
13 years, 11 months ago by t.broyer Modified:
11 years, 7 months ago Base URL:
http://protobuf.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionIn C++, writing a code generator plugin is as easy as implementing CodeGenerator and passing an instance to google::protobuf::compiler::PluginMain; it shouldn't be any harder in Java (or Python BTW).
This patch adds a JavaHelpers class which is just a port of google/protobuf/compiler/java/java_helpers.{h,cc} with a few additional helper methods; and the abstract Plugin class.
To implement a protoc plugin in Java, simply write an implementation of Plugin, then create a main() method like:
public static void main(String[] args) {
new MyPlugin().run(args);
}
The main difference with C++ is that the Java code needs to be launched from a shell/batch script (the patch includes a sample batch file for Windows), and expects that the name of the script be passed as the first command-line argument.
The code currently lacks JavaDoc and unit tests but I'm providing it here for an early review.
Patch Set 1 #Patch Set 2 : Fixes getDefaultValue for enum fields, and parseGeneratorParameter for empty parts #
Total comments: 40
Patch Set 3 : Factored out a CodeGenerator interface, Plugin now has only static methods #
Total comments: 25
Patch Set 4 : javadoc, unit tests, Context#getParsedFiles, streaming to stdout #
Total comments: 21
Patch Set 5 : Few tweaks based on review #
MessagesTotal messages: 16
Fixes getDefaultValue for enum fields, and parseGeneratorParameter for empty parts
Sign in to reply to this message.
Thanks for doing this! This probably should not be part of libprotobuf.jar, since most people don't need it and libprotobuf is too big already. Unfortunately I think the nature of Maven is that we need to create a whole separate project tree for this? http://codereview.appspot.com/912042/diff/6001/7001 File java/protoc-gen-plugin.bat.tmpl (right): http://codereview.appspot.com/912042/diff/6001/7001#newcode1 java/protoc-gen-plugin.bat.tmpl:1: @echo off I'm not sure if I really care to have this file. It's pretty simple (= easy for individual plugin authors to reproduce) and not portable. http://codereview.appspot.com/912042/diff/6001/7002 File java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java (right): http://codereview.appspot.com/912042/diff/6001/7002#newcode16 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:16: public class JavaHelpers { Make final so people don't do something silly like subclass this in order to import all the contents. http://codereview.appspot.com/912042/diff/6001/7002#newcode35 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:35: public static final String THICK_SEPARATOR = "// ===================================================================\n"; Please wrap lines at 80 characters. http://codereview.appspot.com/912042/diff/6001/7002#newcode82 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:82: * @param field Why include @param and @return if you aren't actually filling them in? http://codereview.appspot.com/912042/diff/6001/7002#newcode224 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:224: * Returns the fully-qualified class name corresponding to the given This returns a (static) member name, not a class name. http://codereview.appspot.com/912042/diff/6001/7002#newcode326 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:326: public static String getDefaultValue(FieldDescriptor field) { I doubt plugins need this, and it looks like your implementation is not really complete. For example, you aren't escaping everything that needs to be escaped in strings. Maybe just remove this method? http://codereview.appspot.com/912042/diff/6001/7003 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/6001/7003#newcode21 java/src/main/java/com/google/protobuf/compiler/Plugin.java:21: public abstract class Plugin { This file needs more Javadoc. http://codereview.appspot.com/912042/diff/6001/7003#newcode28 java/src/main/java/com/google/protobuf/compiler/Plugin.java:28: * Remove blank line? http://codereview.appspot.com/912042/diff/6001/7003#newcode35 java/src/main/java/com/google/protobuf/compiler/Plugin.java:35: * It is the caller's responsibility to commit the file by calling the <p> http://codereview.appspot.com/912042/diff/6001/7003#newcode46 java/src/main/java/com/google/protobuf/compiler/Plugin.java:46: // non-private so it's accessible to unit tests You didn't include a unit test. This code should probably have one. http://codereview.appspot.com/912042/diff/6001/7003#newcode50 java/src/main/java/com/google/protobuf/compiler/Plugin.java:50: public int exitWithError(String error) { Remove this method and throw an exception instead wherever you call it. (Define PluginException for this purpose?) http://codereview.appspot.com/912042/diff/6001/7003#newcode67 java/src/main/java/com/google/protobuf/compiler/Plugin.java:67: interface Environment { Nice use of interfaces for testability. Technically you could use System.setIn()/System.setOut() but this is cleaner. http://codereview.appspot.com/912042/diff/6001/7003#newcode106 java/src/main/java/com/google/protobuf/compiler/Plugin.java:106: CodeGeneratorResponse.File.Builder file = CodeGeneratorResponse.File.newBuilder(); Wrap at 80 chars (here and below). http://codereview.appspot.com/912042/diff/6001/7003#newcode138 java/src/main/java/com/google/protobuf/compiler/Plugin.java:138: public static List<Map.Entry<String, String>> parseGeneratorParameter( Why not return an actual Map<String, String>? http://codereview.appspot.com/912042/diff/6001/7003#newcode158 java/src/main/java/com/google/protobuf/compiler/Plugin.java:158: ret.add(new Map.Entry<String, String>() { ret.put(key, value) would be much simpler than this big block you have here... http://codereview.appspot.com/912042/diff/6001/7003#newcode179 java/src/main/java/com/google/protobuf/compiler/Plugin.java:179: public abstract String generate(FileDescriptor file, String parameter, Please use exceptions rather than returning errors. http://codereview.appspot.com/912042/diff/6001/7003#newcode179 java/src/main/java/com/google/protobuf/compiler/Plugin.java:179: public abstract String generate(FileDescriptor file, String parameter, Can you create a separate CodeGenerator interface (similar to the C++ interface of the same name) which the plugin implements, rather than have it subclass Plugin? I am worried that using inheritance here is tying together two things that don't belong is the same object. For example, in C++, the CodeGenerator interface can be called either by the plugin driver or directly by protoc. Maybe someday we'll have a reason to want to do the same with Java code generators. This implies that OutputDirectory and other stuff on which CodeGenerator depends should become top-level classes instead of being nested in Plugin. http://codereview.appspot.com/912042/diff/6001/7003#newcode184 java/src/main/java/com/google/protobuf/compiler/Plugin.java:184: request.getFileToGenerateCount()); getFileToGenerateCount() -> getProtoFileCount()? http://codereview.appspot.com/912042/diff/6001/7003#newcode259 java/src/main/java/com/google/protobuf/compiler/Plugin.java:259: // non-private so it's accessible to unit tests I would actually go ahead and make these public. Tests aren't the only things that can benefit from singleton-free interfaces. http://www.object-oriented-security.org/lets-argue/singletons http://codereview.appspot.com/912042/diff/6001/7003#newcode298 java/src/main/java/com/google/protobuf/compiler/Plugin.java:298: return run(pluginName); Need to pass environment.
Sign in to reply to this message.
Thanks for your review. I've only responded to some comments. I'll update my code and respond to the other comments then. I don't do Maven but my impression is that the files could live in the protobuf repository yet not be compiled and bundled into the libprotobuf.jar using a list of exclusions http://maven.apache.org/plugins/maven-compiler-plugin/compile-mojo.html#excludes I'm expecting Java-based plugin developers to bundle everything into a single JAR, so including the Plugin.java to their codebase shouldn't be an issue. Maybe a Maven profile could be made for plugin developers (just like the "lite" profile) to re-include the previously excluded Plugin, JavaHelpers and PluginProtos. As for the batch file, well, it's just a template to help plugin implementors; and of course a shell script should be provided in due time for unix/linux environments. http://codereview.appspot.com/912042/diff/6001/7002 File java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java (right): http://codereview.appspot.com/912042/diff/6001/7002#newcode16 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:16: public class JavaHelpers { On 2010/04/26 18:45:44, kenton wrote: > Make final so people don't do something silly like subclass this in order to > import all the contents. My plan was to add a private ctor to prevent both subclasses and instantiations but it looks like I forgot it. http://codereview.appspot.com/912042/diff/6001/7002#newcode82 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:82: * @param field On 2010/04/26 18:45:44, kenton wrote: > Why include @param and @return if you aren't actually filling them in? They're autogenerated by Eclipse ;-) Note that the JavaDoc is a plain copy from google::protobuf::compiler::java::java_helpers; I didn't even try fixing or completing it. http://codereview.appspot.com/912042/diff/6001/7002#newcode224 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:224: * Returns the fully-qualified class name corresponding to the given On 2010/04/26 18:45:44, kenton wrote: > This returns a (static) member name, not a class name. Right. I just copied the doc from java_helpers.h down to all following methods: http://code.google.com/p/protobuf/source/browse/trunk/src/google/protobuf/com... http://codereview.appspot.com/912042/diff/6001/7002#newcode326 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:326: public static String getDefaultValue(FieldDescriptor field) { On 2010/04/26 18:45:44, kenton wrote: > I doubt plugins need this, Mine needs it (as I'm generating a different implementation of (a subset of) the Message/MessageLite API); and given that it's part of java_helpers.h public API that C++ plugins could use, I don't see a reason to remove it (and better have it maintained there than having it re-implementing in each and every plugin that needs it). Another use case could be to compare a field's value with its default value to avoid serializing it, making the serialized message smaller at the detriment of code speed. > and it looks like your implementation is not really > complete. For example, you aren't escaping everything > that needs to be escaped in strings. Right! I'll fix this. http://codereview.appspot.com/912042/diff/6001/7003 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/6001/7003#newcode21 java/src/main/java/com/google/protobuf/compiler/Plugin.java:21: public abstract class Plugin { On 2010/04/26 18:45:44, kenton wrote: > This file needs more Javadoc. More generally, everything here needs more JavaDoc and some polishing. But I wanted a first look at the code before spending time documenting it. http://codereview.appspot.com/912042/diff/6001/7003#newcode138 java/src/main/java/com/google/protobuf/compiler/Plugin.java:138: public static List<Map.Entry<String, String>> parseGeneratorParameter( On 2010/04/26 18:45:44, kenton wrote: > Why not return an actual Map<String, String>? To handle repeated parameters. And Map<String,List<String>> would lose relative position of parameters, which a plugin might find important (e.g. "option1=val1,option2=related to val 1,option1=val2,option2=related to val2") List<Map.Entry<String, String>> is the closest I could find from C++ vector<pair<string, string>> (I noticed that google::protobuf::compiler::ParseGeneratorParameter does not use a map<string, string>, there must be a reason?) http://codereview.appspot.com/912042/diff/6001/7003#newcode179 java/src/main/java/com/google/protobuf/compiler/Plugin.java:179: public abstract String generate(FileDescriptor file, String parameter, On 2010/04/26 18:45:44, kenton wrote: > Can you create a separate CodeGenerator interface (similar to the C++ interface > of the same name) which the plugin implements, rather than have it subclass > Plugin? I am worried that using inheritance here is tying together two things > that don't belong is the same object. For example, in C++, the CodeGenerator > interface can be called either by the plugin driver or directly by protoc. > Maybe someday we'll have a reason to want to do the same with Java code > generators. This is actually what I started doing (mimicking the C++ code), but then thought that there would be little to no reason to use CodeGenerator without Plugin in Java (in C++ it's a mtter of whether you want to compile your own modified protoc or use an unmodified one with a plugin) It also has the advantage that you can override the various run() and generate() methods to add some loggin or other things (my code outputs a list of generated files in the .gwt.xml file; the Java generator passes a vector<string> around, but here you could override the run(request,response) to read generated files from the response builder and add a new one (it's in my list of refactorings to do in protobuf-gwt). > This implies that OutputDirectory and other stuff on which CodeGenerator depends > should become top-level classes instead of being nested in Plugin. Either that, or as nested interfaces in the CodeGenerator.
Sign in to reply to this message.
If you don't mind, I'd like to polish/fill-in JavaDoc in a "last call" patchset. http://codereview.appspot.com/912042/diff/6001/7002 File java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java (right): http://codereview.appspot.com/912042/diff/6001/7002#newcode16 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:16: public class JavaHelpers { On 2010/04/26 18:45:44, kenton wrote: > Make final so people don't do something silly like subclass this in order to > import all the contents. Done. http://codereview.appspot.com/912042/diff/6001/7002#newcode35 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:35: public static final String THICK_SEPARATOR = "// ===================================================================\n"; On 2010/04/26 18:45:44, kenton wrote: > Please wrap lines at 80 characters. Done. http://codereview.appspot.com/912042/diff/6001/7002#newcode326 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:326: public static String getDefaultValue(FieldDescriptor field) { On 2010/04/26 22:29:05, t.broyer wrote: > On 2010/04/26 18:45:44, kenton wrote: > > I doubt plugins need this, > > Mine needs it (as I'm generating a different implementation of (a subset of) the > Message/MessageLite API) ...and because I'm generated a different implementation, my needs are a little different (for instance, my ByteString implementation is built around a String, not a byte[], and is therefore constructed from a String), so I'll remove this method. http://codereview.appspot.com/912042/diff/6001/7003 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/6001/7003#newcode50 java/src/main/java/com/google/protobuf/compiler/Plugin.java:50: public int exitWithError(String error) { On 2010/04/26 18:45:44, kenton wrote: > Remove this method and throw an exception instead wherever you call it. (Define > PluginException for this purpose?) Done. http://codereview.appspot.com/912042/diff/6001/7003#newcode106 java/src/main/java/com/google/protobuf/compiler/Plugin.java:106: CodeGeneratorResponse.File.Builder file = CodeGeneratorResponse.File.newBuilder(); On 2010/04/26 18:45:44, kenton wrote: > Wrap at 80 chars (here and below). Done (well, actually at 81; 80 would make the code really ugly) http://codereview.appspot.com/912042/diff/6001/7003#newcode179 java/src/main/java/com/google/protobuf/compiler/Plugin.java:179: public abstract String generate(FileDescriptor file, String parameter, On 2010/04/26 18:45:44, kenton wrote: > Please use exceptions rather than returning errors. Done. http://codereview.appspot.com/912042/diff/6001/7003#newcode179 java/src/main/java/com/google/protobuf/compiler/Plugin.java:179: public abstract String generate(FileDescriptor file, String parameter, On 2010/04/26 18:45:44, kenton wrote: > Can you create a separate CodeGenerator interface (similar to the C++ interface > of the same name) which the plugin implements, rather than have it subclass > Plugin? Done. http://codereview.appspot.com/912042/diff/6001/7003#newcode184 java/src/main/java/com/google/protobuf/compiler/Plugin.java:184: request.getFileToGenerateCount()); On 2010/04/26 18:45:44, kenton wrote: > getFileToGenerateCount() -> getProtoFileCount()? Done. http://codereview.appspot.com/912042/diff/6001/7003#newcode259 java/src/main/java/com/google/protobuf/compiler/Plugin.java:259: // non-private so it's accessible to unit tests On 2010/04/26 18:45:44, kenton wrote: > I would actually go ahead and make these public. Tests aren't the only things > that can benefit from singleton-free interfaces. > > http://www.object-oriented-security.org/lets-argue/singletons Done. http://codereview.appspot.com/912042/diff/6001/7003#newcode298 java/src/main/java/com/google/protobuf/compiler/Plugin.java:298: return run(pluginName); On 2010/04/26 18:45:44, kenton wrote: > Need to pass environment. I actually refactored the whole thing and got rid of the "plugin name", which is actually not that helpful so not worth the work. A plugin would now just call Plugin.run(new MyPlugin()). If there's a need to process command-line arguments, then it would initialize/configure the code generator priori to passing it to Plugin.run().
Sign in to reply to this message.
Factored out a CodeGenerator interface, Plugin now has only static methods
Sign in to reply to this message.
Next patchset will add unit test too. http://codereview.appspot.com/912042/diff/22001/23001 File java/protoc-gen-plugin.bat.tmpl (right): http://codereview.appspot.com/912042/diff/22001/23001#newcode11 java/protoc-gen-plugin.bat.tmpl:11: Oops! just noticed a huge bug here, where it'll fall back to "err" on successful invocation.
Sign in to reply to this message.
I've CC'd Gregory Kick, the Maven expert. Greg, this patch is contributing a library for implementing protoc compiler plugins in Java. This library depends on libprotobuf, but only a small minority of users need this extra code, so we don't want to put it into libprotobuf itself. We probably want to build it as a separate "libprotoc.jar". What's the proper way to set this up? Other notes: * Still missing some doc comments. * I still would rather not have the .bat and .sh files. It seems that these are just templates that you intend for developers to copy and extend. Isn't this already a common problem in Java land? Aren't there existing solutions? I'd rather not offer our own thing which may or may not work correctly. (I made some comments on these scripts below just as advice for you, since I assume you'll be using these for your own plugin.) http://codereview.appspot.com/912042/diff/6001/7002 File java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java (right): http://codereview.appspot.com/912042/diff/6001/7002#newcode82 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:82: * @param field On 2010/04/26 22:29:05, t.broyer wrote: > On 2010/04/26 18:45:44, kenton wrote: > > Why include @param and @return if you aren't actually filling them in? > > They're autogenerated by Eclipse ;-) > > Note that the JavaDoc is a plain copy from > google::protobuf::compiler::java::java_helpers; I didn't even try fixing or > completing it. Please delete the annotations that you have not filled in. (I think it's fine not to fill them in, but empty annotations just waste space.) http://codereview.appspot.com/912042/diff/6001/7002#newcode224 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:224: * Returns the fully-qualified class name corresponding to the given On 2010/04/26 22:29:05, t.broyer wrote: > On 2010/04/26 18:45:44, kenton wrote: > > This returns a (static) member name, not a class name. > > Right. I just copied the doc from java_helpers.h down to all following methods: > http://code.google.com/p/protobuf/source/browse/trunk/src/google/protobuf/com... OK, then it's my fault. Please fix it anyway; there's no reason to retain incorrect docs. http://codereview.appspot.com/912042/diff/6001/7002#newcode326 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:326: public static String getDefaultValue(FieldDescriptor field) { On 2010/05/17 14:30:09, t.broyer wrote: > On 2010/04/26 22:29:05, t.broyer wrote: > > On 2010/04/26 18:45:44, kenton wrote: > > > I doubt plugins need this, > > > > Mine needs it (as I'm generating a different implementation of (a subset of) > the > > Message/MessageLite API) > > ...and because I'm generated a different implementation, my needs are a little > different (for instance, my ByteString implementation is built around a String, > not a byte[], and is therefore constructed from a String), so I'll remove this > method. OK. But, how do you build a ByteString around a String? That doesn't make sense -- Strings cannot represent raw bytes, unless you're escaping them somehow, which would be inefficient. http://codereview.appspot.com/912042/diff/22001/23002 File java/protoc-gen-plugin.sh.tmpl (right): http://codereview.appspot.com/912042/diff/22001/23002#newcode8 java/protoc-gen-plugin.sh.tmpl:8: pushd `dirname $0` You don't need to use pushd/popd. Unix shell scripts run in their own process and do not affect the current working directory of the parent process. Note that pushd/popd are bash-specific, so you can't use them in portable shell scripts. http://codereview.appspot.com/912042/diff/22001/23002#newcode9 java/protoc-gen-plugin.sh.tmpl:9: java -jar myplugin.jar You can just do: exec java -jar myplugin.jar This causes the "java" process to replace the shell script, so the exit code it returns will be returned to the script's original caller. So, you can then delete the rest of the lines of the script. http://codereview.appspot.com/912042/diff/22001/23003 File java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java (right): http://codereview.appspot.com/912042/diff/22001/23003#newcode9 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:9: public interface OutputDirectory { Rename OutputDirectory to Context -- we're planning to rename the C++ interface similarly in the next release. Also add a method: List<FileDescriptor> ListParsedFiles(); It should return all the files for which the plugin needs to generate code. This can be used to fix the bug you observed with the Java generator's manifest file, and is also needed by the Go code generator. The next release of the protobufs will have this method in the C++ interface as well. http://codereview.appspot.com/912042/diff/22001/23003#newcode15 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:15: * Remove blank line. http://codereview.appspot.com/912042/diff/22001/23003#newcode22 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:22: * It is the caller's responsibility to commit the file by calling the <p> http://codereview.appspot.com/912042/diff/22001/23003#newcode25 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:25: * @param filename Remove unfilled annotations. http://codereview.appspot.com/912042/diff/22001/23005 File java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java (right): http://codereview.appspot.com/912042/diff/22001/23005#newcode190 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:190: // Java package. Put this text on the end of the previous line. http://codereview.appspot.com/912042/diff/22001/23005#newcode418 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:418: // capitalization of the type name. Append to previous line. http://codereview.appspot.com/912042/diff/22001/23005#newcode442 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:442: // capitalize it. Append to previous line. http://codereview.appspot.com/912042/diff/22001/23005#newcode462 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:462: public static List<Map.Entry<String, String>> parseGeneratorParameter( Again, please return an actual Map, not a List of Map.Entry. There is no need to support repeated parameters. The parameter list is intuitively a map, so repeated parameters would just confuse the user anyway. Might as well disallow them. The C++ code uses vector<pair<string, string> > for no particularly good reason. If there were a Java equivalent to pair, I might say to use that. Map.Entry is not equivalent -- it's explicitly part of the Map interface. Also, constructing anonymous classes to implement it is awkward. It would be both easier and more intuitive to just return a Map. Exactly matching the C++ interface is not necessary. http://codereview.appspot.com/912042/diff/22001/23006 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/22001/23006#newcode22 java/src/main/java/com/google/protobuf/compiler/Plugin.java:22: public static class DefaultEnvironment implements Plugin.Environment { Please keep everything private unless it actually has to be public. I don't see any reason why third-parties need to use DefaultEnvironment, GeneratorResponseOutputDirectory, etc. http://codereview.appspot.com/912042/diff/22001/23006#newcode56 java/src/main/java/com/google/protobuf/compiler/Plugin.java:56: private final CodeGeneratorResponse.Builder response_; Please remove the underscore -- this violates Java style rules. http://codereview.appspot.com/912042/diff/22001/23006#newcode118 java/src/main/java/com/google/protobuf/compiler/Plugin.java:118: public static void run(CodeGenerator generator) throws GeneratorException { Perhaps this version of run() should catch GeneratorExceptions and print the error (without a stack trace) to System.err. http://codereview.appspot.com/912042/diff/22001/23006#newcode122 java/src/main/java/com/google/protobuf/compiler/Plugin.java:122: public static void run(CodeGenerator generator, CodeGeneratorRequest request, Re-order this to be after run(CodeGenerator,Environment). That way each overload of run() calls the one after it, making things easier to read. http://codereview.appspot.com/912042/diff/22001/23006#newcode130 java/src/main/java/com/google/protobuf/compiler/Plugin.java:130: dependencies[i] = filesByName.get(protoFile.getDependency(i)); Throw GeneratorException if result is null.
Sign in to reply to this message.
The OutputDirectory->Context move looks good, except for the "ListParsedFiles" method return type (see comments in CodeGenerator.java) Next patch set will include doc and unit tests, though I'd like to here from you on the above issue (I've implemented the List<String> rather than List<FileDescriptor> for now). http://codereview.appspot.com/912042/diff/22001/23002 File java/protoc-gen-plugin.sh.tmpl (right): http://codereview.appspot.com/912042/diff/22001/23002#newcode8 java/protoc-gen-plugin.sh.tmpl:8: pushd `dirname $0` On 2010/05/21 02:40:36, kenton wrote: > You don't need to use pushd/popd. Unix shell scripts run in their own process > and do not affect the current working directory of the parent process. > > Note that pushd/popd are bash-specific, so you can't use them in portable shell > scripts. Thanks for the tips http://codereview.appspot.com/912042/diff/22001/23003 File java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java (right): http://codereview.appspot.com/912042/diff/22001/23003#newcode9 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:9: public interface OutputDirectory { On 2010/05/21 02:40:36, kenton wrote: > Rename OutputDirectory to Context -- we're planning to rename the C++ interface > similarly in the next release. > > Also add a method: > List<FileDescriptor> ListParsedFiles(); > > It should return all the files for which the plugin needs to generate code. > This can be used to fix the bug you observed with the Java generator's manifest > file, and is also needed by the Go code generator. The next release of the > protobufs will have this method in the C++ interface as well. Done. Except I'd rather name the method "getParsedFiles" (or getFilesToGenerate) than "ListParsedFiles". Though in retrospect, wouldn't it rule out the potential optimization where the plugin would stream the CodeGeneratorRequest from stdin? A List<String>, equivalent to the CodeGeneratorRequest.file_to_generate repeated field, would allow streaming the proto_file repeated field while making it possible to fix the bug in the Java generator re the manifest file. If a plugin really needs all the FileDescriptors, it could save them in each call to generate() until the last (easily detectable: either you count them and compare to getFilesToGenerate().size(), or you compare their names to the items in getFilesToGenerate()) and only then start to generate files. A bit tricky, but would allow making the plugin infrastructure more efficient later by streaming from stdin (and to stdout). http://codereview.appspot.com/912042/diff/22001/23005 File java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java (right): http://codereview.appspot.com/912042/diff/22001/23005#newcode462 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:462: public static List<Map.Entry<String, String>> parseGeneratorParameter( On 2010/05/21 02:40:36, kenton wrote: > Again, please return an actual Map, not a List of Map.Entry. Done. http://codereview.appspot.com/912042/diff/22001/23006 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/22001/23006#newcode22 java/src/main/java/com/google/protobuf/compiler/Plugin.java:22: public static class DefaultEnvironment implements Plugin.Environment { On 2010/05/21 02:40:36, kenton wrote: > Please keep everything private unless it actually has to be public. I don't see > any reason why third-parties need to use DefaultEnvironment, > GeneratorResponseOutputDirectory, etc. Done. http://codereview.appspot.com/912042/diff/22001/23006#newcode56 java/src/main/java/com/google/protobuf/compiler/Plugin.java:56: private final CodeGeneratorResponse.Builder response_; On 2010/05/21 02:40:36, kenton wrote: > Please remove the underscore -- this violates Java style rules. Done. http://codereview.appspot.com/912042/diff/22001/23006#newcode118 java/src/main/java/com/google/protobuf/compiler/Plugin.java:118: public static void run(CodeGenerator generator) throws GeneratorException { On 2010/05/21 02:40:36, kenton wrote: > Perhaps this version of run() should catch GeneratorExceptions and print the > error (without a stack trace) to System.err. It would have to explicitly System.exit(1) then, which I don't quite like. If a plugin developer wants, for whichever reason, to do some work after the call to run(), he would have to use the run(CodeGenerator,Environment) overload, which means he would have to re-implement DefaultEnvironment (because we moved it to private above). Because the GeneratorException here means protoc failed to send the appropriate request and/or to communicate with the plugin (or a bug in the .bat/.sh script sitting in between protoc and java), my plugin just has a "throws GeneratorException" on its main() method. If it ever happens, the JVM will print the exception (with stack trace, to help debug the error) and exit with a non-zero exit code (and protoc then knows the generation failed). It looks to me like a good compromise. http://codereview.appspot.com/912042/diff/22001/23006#newcode130 java/src/main/java/com/google/protobuf/compiler/Plugin.java:130: dependencies[i] = filesByName.get(protoFile.getDependency(i)); On 2010/05/21 02:40:36, kenton wrote: > Throw GeneratorException if result is null. Done.
Sign in to reply to this message.
http://codereview.appspot.com/912042/diff/22001/23003 File java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java (right): http://codereview.appspot.com/912042/diff/22001/23003#newcode9 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:9: public interface OutputDirectory { On 2010/05/22 23:30:46, t.broyer wrote: > > Except I'd rather name the method "getParsedFiles" (or getFilesToGenerate) than > "ListParsedFiles". > > Though in retrospect, wouldn't it rule out the potential optimization where the > plugin would stream the CodeGeneratorRequest from stdin? After some more thinking (and trial: I started implementing streaming of input just to prove it'd be possible) it appears you'll still have to have all FileDescriptorProtos in memory at some point in time (when processing the very last file_proto, you need all previous file_proto –or FileDescriptor, but the FileDescriptorProto is kept in memory in a FileDescriptor field– because you don't know which dependencies there would be). In other words, streaming input only allows beginning code generation while still reading input, which is probably not worth it (streaming the output though is another story as it allows reclaiming memory instead of building the whole CodeGeneratorResponse and flushing it at the very end).
Sign in to reply to this message.
javadoc, unit tests, Context#getParsedFiles, streaming to stdout
Sign in to reply to this message.
The 4th patch: - adds javadoc - adds unit tests - renames OutputDirectory to Context - adds Context#getParsedFiles - renames Environment's getStandardXxx() methods to getXxxStream() - writes generated files to the Environment#getOutputStream as soon as they are "committed" instead of building the full CodeGeneratorResponse in memory. This takes advantage of the protocol binary format where concatenating serialized messages is equivalent to merging them while they are being deserialized - and removes the launcher script templates (highly simplified and moved to the javadoc of the Plugin class) http://codereview.appspot.com/912042/diff/22001/23006 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/22001/23006#newcode22 java/src/main/java/com/google/protobuf/compiler/Plugin.java:22: public static class DefaultEnvironment implements Plugin.Environment { On 2010/05/22 23:30:46, t.broyer wrote: > On 2010/05/21 02:40:36, kenton wrote: > > Please keep everything private unless it actually has to be public. I don't > see > > any reason why third-parties need to use DefaultEnvironment, > > GeneratorResponseOutputDirectory, etc. > > Done. I finally moved DefaultEnvironment back to public scope, given how trivial it is. http://codereview.appspot.com/912042/diff/40001/41001 File java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java (right): http://codereview.appspot.com/912042/diff/40001/41001#newcode86 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:86: public static class GeneratorException extends Exception { Now that Plugin no longer throws GeneratorException (see comments in Plugin.java), I moved it here, alongside Context, to make it clear that it's specific to CodeGenerators. http://codereview.appspot.com/912042/diff/40001/41003 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/40001/41003#newcode76 java/src/main/java/com/google/protobuf/compiler/Plugin.java:76: public static class PluginException extends RuntimeException { I've introduced this RuntimeException to support streaming generated files to stdout. It has to be a RuntimeException to not introduce a dependency from CodeGenerator on Plugin (we would have to add it as a checked exception in CodeGenerator#generate; and we need a distinct exception to tell stdout errors from generator errors). This in turns make the Plugin infrastructure a bit easier to understand, as before that a GeneratorException thrown by a generator would be handled by the plugin into the CodeGeneratorResponse#error while GeneratorExceptions that would propagate out from the Plugin#run methods would only be ones thrown by the Plugin methods themselves ("I pass a CodeGenerator that throws a GeneratorException but it never propagates out of Plugin#run, WTF!" Now it's clearer, and I added throws clauses to Plugin methods to make it even clearer). http://codereview.appspot.com/912042/diff/40001/41003#newcode127 java/src/main/java/com/google/protobuf/compiler/Plugin.java:127: static abstract class AbstractContext implements CodeGenerator.Context { To keep the run(CodeGenerator,CodeGeneratorRequest,CodeGeneratorResponse.Builder) method while allowing streaming files to stdout otherwise (when using run(CodeGenerator) or run(CodeGenerator,Environment)), I had to introduce two distinct Context implementations, which share a common base. These classes are package-scope to be visible from unit tests.
Sign in to reply to this message.
Looking pretty good. http://codereview.appspot.com/912042/diff/40001/41001 File java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java (right): http://codereview.appspot.com/912042/diff/40001/41001#newcode35 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:35: * The generator's {@link CodeGenerator#generate(FileDescriptor, String, Context)} 80 char limit http://codereview.appspot.com/912042/diff/40001/41001#newcode36 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:36: * method will be called once for each file in the list. Add: "Most generators do not need to call getParsedFiles()." http://codereview.appspot.com/912042/diff/40001/41001#newcode110 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:110: final String getErrorMessage() { This doesn't really seem necessary. I'd just require that a message always be provided. http://codereview.appspot.com/912042/diff/40001/41002 File java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java (right): http://codereview.appspot.com/912042/diff/40001/41002#newcode31 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:31: public static final String OUTER_CLASS_SCOPE = "outer_class_scope"; OUTER_CLASS_SCOPE_INSERTION_POINT? Or maybe make it a method just for consistency with the others... getOuterClassScopeInsertionPoint() http://codereview.appspot.com/912042/diff/40001/41003 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/40001/41003#newcode43 java/src/main/java/com/google/protobuf/compiler/Plugin.java:43: * java -jar "%~dp0\myplugin.jar" What's %~dp0? http://codereview.appspot.com/912042/diff/40001/41003#newcode44 java/src/main/java/com/google/protobuf/compiler/Plugin.java:44: * if errorlevel 1 exit %ERRORLEVEL% Could this just be "exit %ERRORLEVEL%"? http://codereview.appspot.com/912042/diff/40001/41003#newcode273 java/src/main/java/com/google/protobuf/compiler/Plugin.java:273: throw new PluginException("protoc sent unparseable request to plugin.", e); 80 char limit http://codereview.appspot.com/912042/diff/40001/41003#newcode319 java/src/main/java/com/google/protobuf/compiler/Plugin.java:319: public static void generate(CodeGenerator generator, String parameter, Why public? This is a trivial loop; doesn't seem like it needs to be part of the interface. http://codereview.appspot.com/912042/diff/40001/41003#newcode321 java/src/main/java/com/google/protobuf/compiler/Plugin.java:321: throws GeneratorException { Append to previous line -- this indentation looks weird otherwise.
Sign in to reply to this message.
http://codereview.appspot.com/912042/diff/40001/41001 File java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java (right): http://codereview.appspot.com/912042/diff/40001/41001#newcode35 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:35: * The generator's {@link CodeGenerator#generate(FileDescriptor, String, Context)} On 2010/05/26 23:21:07, kenton wrote: > 80 char limit Done. http://codereview.appspot.com/912042/diff/40001/41001#newcode36 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:36: * method will be called once for each file in the list. On 2010/05/26 23:21:07, kenton wrote: > Add: "Most generators do not need to call getParsedFiles()." Done, along with a note about the list ordering (that it's stable among calls to CodeGenerator#generate, so you can determine for instance whether you're processing the last (or first) file and thus generate a single file for the whole "context" scope, e.g. the list of generated files. http://codereview.appspot.com/912042/diff/40001/41001#newcode110 java/src/main/java/com/google/protobuf/compiler/CodeGenerator.java:110: final String getErrorMessage() { On 2010/05/26 23:21:07, kenton wrote: > This doesn't really seem necessary. I'd just require that a message always be > provided. Does it mean the class should be marked final? (or only the getMessage() method maybe?) with checks added to the constructors? (throwing InvalidArgumentException or NullPointerException from within the constructor?) Note that in the GeneratorException(Throwable) case it'd mean cause is not null but also cause.getMessage(). Or do you mean that Plugin should just use getMessage() and let CodeGeneratorResponse.Builder#setError throw? That's what I ended up doing. http://codereview.appspot.com/912042/diff/40001/41002 File java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java (right): http://codereview.appspot.com/912042/diff/40001/41002#newcode31 java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java:31: public static final String OUTER_CLASS_SCOPE = "outer_class_scope"; On 2010/05/26 23:21:07, kenton wrote: > OUTER_CLASS_SCOPE_INSERTION_POINT? > > Or maybe make it a method just for consistency with the others... > getOuterClassScopeInsertionPoint() Done. http://codereview.appspot.com/912042/diff/40001/41003 File java/src/main/java/com/google/protobuf/compiler/Plugin.java (right): http://codereview.appspot.com/912042/diff/40001/41003#newcode43 java/src/main/java/com/google/protobuf/compiler/Plugin.java:43: * java -jar "%~dp0\myplugin.jar" On 2010/05/26 23:21:07, kenton wrote: > What's %~dp0? It's %0 (i.e. the batch file name) expanded to the full path with drive letter. http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-u... http://codereview.appspot.com/912042/diff/40001/41003#newcode44 java/src/main/java/com/google/protobuf/compiler/Plugin.java:44: * if errorlevel 1 exit %ERRORLEVEL% On 2010/05/26 23:21:07, kenton wrote: > Could this just be "exit %ERRORLEVEL%"? I was surprised it didn't work when I tested it, but I tested again (just in case) and it worked. I assume my first test was broken. http://codereview.appspot.com/912042/diff/40001/41003#newcode273 java/src/main/java/com/google/protobuf/compiler/Plugin.java:273: throw new PluginException("protoc sent unparseable request to plugin.", e); On 2010/05/26 23:21:07, kenton wrote: > 80 char limit Done, here and at some other places where the line was 81 chars long. http://codereview.appspot.com/912042/diff/40001/41003#newcode319 java/src/main/java/com/google/protobuf/compiler/Plugin.java:319: public static void generate(CodeGenerator generator, String parameter, On 2010/05/26 23:21:07, kenton wrote: > Why public? This is a trivial loop; doesn't seem like it needs to be part of > the interface. Done. http://codereview.appspot.com/912042/diff/40001/41003#newcode321 java/src/main/java/com/google/protobuf/compiler/Plugin.java:321: throws GeneratorException { On 2010/05/26 23:21:07, kenton wrote: > Append to previous line -- this indentation looks weird otherwise. Done.
Sign in to reply to this message.
Few tweaks based on review
Sign in to reply to this message.
Did this change ever get merged in? It turns out this is pretty much exactly what I need for writing a Java-based plugin. On Wednesday, April 14, 2010 6:15:30 AM UTC-4, Thomas Broyer wrote: > > Reviewers: kenton, > > Description: > In C++, writing a code generator plugin is as easy as implementing > CodeGenerator and passing an instance to > google::protobuf::compiler::PluginMain; it shouldn't be any harder in > Java (or Python BTW). > > This patch adds a JavaHelpers class which is just a port of > google/protobuf/compiler/java/java_helpers.{h,cc} with a few additional > helper methods; and the abstract Plugin class. > > To implement a protoc plugin in Java, simply write an implementation of > Plugin, then create a main() method like: > public static void main(String[] args) { > new MyPlugin().run(args); > } > > The main difference with C++ is that the Java code needs to be launched > from a shell/batch script (the patch includes a sample batch file for > Windows), and expects that the name of the script be passed as the first > command-line argument. > > The code currently lacks JavaDoc and unit tests but I'm providing it > here for an early review. > > Please review this at http://codereview.appspot.com/912042/show > > Affected files: > java/protoc-gen-plugin.bat.tmpl > java/src/main/java/com/google/protobuf/compiler/JavaHelpers.java > java/src/main/java/com/google/protobuf/compiler/Plugin.java > src/google/protobuf/compiler/plugin.proto > > >
Sign in to reply to this message.
|