search

xolstice / protobuf-maven-plugin

Maven Plugin that executes the Protocol Buffers (protoc) compiler
https://www.xolstice.org/protobuf-maven-plugin/
Other
232 stars 76 forks source link

Add useArgumentFile option to help with Command line is too long error #42

Closed cheister closed 6 years ago

cheister commented 6 years ago

This is primarily to address https://github.com/xolstice/protobuf-maven-plugin/issues/5

I've only tested it on OSX and Linux. I don't have a Windows setup to test with.

A nicer option might be to detect if protoc is 3.5.0 or higher and then automatically put the args in the argumentFile but I wasn't sure how to get the protoc version in a nice way.

sergei-ivanov commented 6 years ago

Thanks for that, I think that should be good enough for everyone. I am going to update the documentation and FAQ. I've tested it on Windows, and there have been no regressions, but we'll also need to add a couple of integration tests in order to verify the new functionality.

cheister commented 6 years ago

SG. I added one basic integration test for the useArgumentFile option.

sergei-ivanov commented 6 years ago

Don’t worry, I have created one last night, I only need to retest the whole thing on Linux. Thank you for your contribution!

Kind regards, Sergey Ivanov

On 10 May 2018, at 07:56, cheister notifications@github.com wrote:

SG. I add one basic integration test for the useArgumentFile option.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

sergei-ivanov commented 6 years ago

Hi,

I've pushed my changes to your PR branch. I've tested them on both Windows and Linux, and the tests pass. I ended up removing path normalisation, because everything works without it on both OSes. I have also added better handling of unicode output and unicode file names (covered by the new integration test too).

Can you please test it on MacOS by running:

./mvnw clean verify -P run-its -Dinvoker.test='setup-*',TEST-29

If that works, we are good. I'll merge it as soon as you let me know that you're happy with the changes.

Sergey

cheister commented 6 years ago

The changes look good to me.

The above command also works on OSX (10.13) for me.