Skip to content

Support --descriptor_set_out / --include_imports#15

Open
binkley wants to merge 13 commits intodtrott:masterfrom
binkley:master
Open

Support --descriptor_set_out / --include_imports#15
binkley wants to merge 13 commits intodtrott:masterfrom
binkley:master

Conversation

@binkley
Copy link

@binkley binkley commented Sep 26, 2011

I've added support for --descriptor_set_out and --include_imports, among other small changes. Please see my README for details. I made as minimal changes as possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole if{} block should be moved inside buildProtocCommand() method for consistency

@sergei-ivanov
Copy link

Adding target/classes is probably unnecessary, I think the only reason you have to do it is because the original plugin incorrectly attaches "namespaced" protos to the resources. Effectively, it flattens the nested directory structure for source protos. I was going to fix it on my branch, I had to disable a bunch of integration tests that failed because of that plugin bug. Warning: it may be a potentially breaking change, but I hope nobody out there is relying on the incorrect behaviour.

UPDATE: It looks like it's indeed a separate problem. I created a couple of integration tests to see if compile scoped protos can be included into test scoped protos. Apparently we'll need to add some sort of extension point to cater for this scenario in the protoc plugin. I'd like to research if compile output paths are already provided by Maven project in some way. If they are, it would be better to use the internal mechanisms instead of injecting target/classes via a property.

@sergei-ivanov
Copy link

A general comment is the same as on the pull request #13. I propose that we split out mojos for generating non-java files. This will allow for cleaner separation, flexible configuration and sensible defaults. The plugin users will have to define additional plugin executions if needed, which is more in a "Maven way" of doing things. Basic configuration can still be shared between mojos and their respective executions.

I think it is great that more people started to contribute to the plugin, but we need to make sure that we are not going to create a mess out of it by trying to cater for all possible usage scenarios in one place.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants