Proto generation spreading #174
Conversation
|
I understand you are currently experiencing trouble in Windows. I would rather have this issue dealt with in the implementation without adding a new option, given that protocolbuffers/protobuf/issues/274 should be the appropriate fix and this knob will only be temporary. It's easy to add a new option but very difficult to remove one. Can you figure out the actual limit on Windows, and just adapt the command line to it? Something like this: int cmdLimit = Integer.MAX_INT;
if (isWindows()) {
cmdLimit = WINDOWS_CMD_LIMIT;
}
// In a loop, construct the command line by adding
// proto file one by one until it reaches cmdLimit, then
// start a new command line, and so on. |
|
@zhangkun83 Thanks for the feedback. I had considered this, but wasn't sure if it was the right way to go about it. |
| currentCmd.append(proto.toString()).append(" ") | ||
| currentFileCount++ | ||
| // Offset to account for trailing space | ||
| if (currentCmd.length() - 1 > lengthLimit) { |
There was a problem hiding this comment.
This won't prevent currentCmd from exceeding the limit at the beginning of the next iteration, and -1 should be +1.
The condition should be currentCmd.length() + 1 + nextProtoFile.toString().length() > lengthLimit
| currentCmd.setLength(baseCmd.length()) | ||
| } | ||
| } | ||
| if (currentFileCount > 0) { |
There was a problem hiding this comment.
This only executes the last command. It won't work.
|
|
||
| String baseCmd = cmd.join(" ") + " " | ||
|
|
||
| StringBuilder currentCmd = new StringBuilder(baseCmd) |
There was a problem hiding this comment.
I think a unit test for this logic is necessary. To make this testable, consider defining a static method like this:
List<String> generateCmds(String baseCmd, List<File> protoFiles, int cmdLineLimit)|
|
||
| then: "it splits appropriately" | ||
| cmds.size() == 2 | ||
| } |
There was a problem hiding this comment.
Please also add a case where it doesn't split.
| return cmds | ||
| } | ||
|
|
||
| private static int getCmdLengthLimit() { |
There was a problem hiding this comment.
Please also split out the actual handling logic, something like getCmdLengthLimit(os, version), and add a test for it.
| List<String> cmds = GenerateProtoTask.generateCmds(baseCmd, protoFiles, cmdLengthLimit) | ||
|
|
||
| then: "it splits appropriately" | ||
| cmds.size() == 2 |
There was a problem hiding this comment.
One last comment: should be better to assert the actual list content rather than the size.
Spread proto generation over multiple protoc calls on systems that limit command line length, e.g., Windows. Once the command length exceeds the OS limit, it calls with the current arguments and resets the builder. This is related to google#167, allowing this issue to be avoided when many proto files are present, until protocolbuffers/protobuf#274 is completed.
This PR spreads proto generation over multiple
protoccalls when on Windows. Once the command length exceeds the OS limit, it calls with the current arguments and resets the builder.This is related to #167, allowing this issue to be avoided when many proto files are present, until protocolbuffers/protobuf#274 is completed.