Skip to content
This repository has been archived by the owner on May 20, 2024. It is now read-only.

Fix protoc java stub generation #6

Merged
merged 1 commit into from
Nov 2, 2018
Merged

Conversation

szab100
Copy link
Contributor

@szab100 szab100 commented Sep 13, 2018

No description provided.

Copy link

@dturbay dturbay left a comment

Choose a reason for hiding this comment

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

Why is this fix for? What is the issue?

pom.xml Outdated
<pluginExecutionFilter>
<groupId>kr.motd.maven</groupId>
<artifactId>os-maven-plugin</artifactId>
<versionRange>[1.5.0.Final,)</versionRange>
Copy link

Choose a reason for hiding this comment

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

I believe fixed version is the best option. With concrete version for all deps/plugins you can have reproducible builds.

Copy link
Contributor

@PapaJoltOmega PapaJoltOmega left a comment

Choose a reason for hiding this comment

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

#1 What was wrong with proto java stub generation? It seemed to work for me.
#2 Does the commit description have any info on the changes related to eclipse? Can the eclipse related changes be in a seperate CL?

Copy link
Contributor

@PapaJoltOmega PapaJoltOmega left a comment

Choose a reason for hiding this comment

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

So what does this CL change about generation?

</execution>
</executions>
</plugin>
<!-- generate protobuffer java classes -->
<!-- Generate protobuffer java stub -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Are they reallly stubs? Where is the actual generated Java code?

</execution>
</executions>
</plugin>
<!-- generate protobuffer java classes -->
<!-- Generate protobuffer java stub -->
Copy link
Contributor

Choose a reason for hiding this comment

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

is the case change your own personal pref? it seems like whoever wrote the comment first had pref for lower case. in general don't create unnecessary churn in CLs, please.

@szab100
Copy link
Contributor Author

szab100 commented Sep 13, 2018

This CL fixes the following:

  • the .proto files were located into java source folders, moved them into src/main/proto as per specification of maven-protoc plugin
  • the os-maven-plugin was accidentally added twice to pom.xml, only one is required
  • Eclipse was not able to run the protoc stub generation, because the os-maven-plugin maven extension was not executed by m2e (eclipse maven integration). Fortunately, the os-maven-plugin can be used as a maven plugin (rather than an extension). The plugin's 'detect' goal is now hooked into the earliest 'initialize' maven phase. In order to allow m2e to execute this execution hook, we need to add this m2e pom.xml snippet. This way eclipse runs os-maven-plugin on build, therefore resolving the os-architecture property and the protoc compiler plugin can generate the java stub sources. Reference to lifecycle mappings on eclipse/m2e page: http://www.eclipse.org/m2e/documentation/m2e-execution-not-covered.html

@dturbay
Copy link

dturbay commented Sep 13, 2018

Ok, I understand the issue now but for me this cl is still controversial.You added kind of hack for an eclipse (1 of many IDEs).

  • Have you tried this plugin in Intellij Idea? May be we have to add some hacks for idea too.
  • Is it possible to build project outside of eclipse and still work in eclipse?
  • Is it possible to configure eclipse to disable that hook security restriction?

@szab100
Copy link
Contributor Author

szab100 commented Sep 24, 2018

Ok, I understand the issue now but for me this cl is still controversial.You added kind of hack for an eclipse (1 of many IDEs).

Well, like it or not, Eclipse still seems to be (by far) the top (java) IDE: https://trends.google.com/trends/explore?geo=US&q=%2Fm%2F01fs1d,%2Fm%2F03v0mn,%2Fm%2F01fchg

And I wouldn't say it's a hack, it's an extra configuration for Eclipse. Other than adding a few more jars downloaded by Maven, it has no effect on a console maven build or using with other IDEs.

  • Have you tried this plugin in Intellij Idea? May be we have to add some hacks for idea too.

Idea's Maven integration has less features than Eclipse's, it is basically just calling out to the command line maven utility. According to my tests, it works with the os-detect plugin out of the box, if the developer runs a maven build explicitly from Idea.

  • Is it possible to build project outside of eclipse and still work in eclipse?

Yes, it works, this pom entry has no effect on non-eclipse scenarios.

  • Is it possible to configure eclipse to disable that hook security restriction?

I don't know, It might be possible, but could be very complicated (m2e maven integration can be configured under .eclipse/plugins/..../../../ configuration files..). With this extra configuration in maven, allowing Eclipse to run this hook my intention is to make it simple and easy for Eclipse users to import and work with this project. In other words, to avoid complicated custom eclipse configurations and detailed instructions.

@szab100
Copy link
Contributor Author

szab100 commented Sep 26, 2018

Update: There is a good chance that the m2e lifecycle config will be added to the 'os-maven-detect' plugin itself (into its pom.xml), so the users (incl. us) won't need to include it. But it's unsure if it works that way.. See my conversation on the plugin's GH repo here: xolstice/protobuf-maven-plugin#10 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants