-
Notifications
You must be signed in to change notification settings - Fork 870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Eclipse plugin build #465
Conversation
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
This PR was initially created before the move to building with Java 11. After rebasing, the build fails due to the maven javadoc build with the following error:
Even with the failure, the plugin jar is still build and seems to be completely functional. (And, if I remove the doc build, everything builds fine.) I had a look at recent changes and found the adjustment to the core Any idea how to fix the javadoc build? |
Move to JDK 15 - it contains a fix for https://bugs.openjdk.java.net/browse/JDK-8240169 |
@googlebot I fixed it. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@sormuras I am not really sure what you mean. As far as I can tell, the google java format core is also build using Java 11 and the javadoc build seems to work just fine. Only the javadoc build for the Eclipse plugin fails (i.e. if I disable the Eclipse build, the build passes). Furthermore, wouldn't this set the minimal required Java version to 15 (which hasn't even been released yet)? |
The underlying problem for why "javadoc build for the Eclipse plugin fails" is due to the issue linked above. Upgrading to 15-ea could solve the problem, introduce other ones ... and is not an option today. So, I guess, you have figure out the differences in how the core project and the Eclipse plugin configure their |
As far as I can tell, the javadoc run is configured in the Would simply skipping the maven javadoc build for the eclipse plugin be an acceptable solution? This can be easily done by adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses at least 4 separate things at once:
- documenting the JDK 11 requirement in the README → should address ErrorProne as a whole, not just the Eclipse plugin
- change the way version is handled in the build (through
${revision}
) → breaks deployment - building the Eclipse plugin when building GJF itself →if the Eclipse plugin uses the "public API" of GJF, it should IMO be considered a separate project and build against the latest released version of GJF (it should be possible to easily build a version of the plugin that uses a snapshot of GJF, but that shouldn't be the default behavior). The plugin might also need intermediate releases independent of GJF releases (due to bugs, or changes in Eclipse), so it shouldn't share the same version as the reactor (see also point above) and should build against a stable GJF version by default.
- fixing the Eclipse plugin build (update Tycho version and configure m-compiler-p to target JDK 11)
At least some of those should be split into separate PRs IMO (or abandoned entirely, if you ask me)
eclipse_plugin/pom.xml
Outdated
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-compiler-plugin</artifactId> | ||
<configuration> | ||
<source>11</source> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<release>11</release>
would be better, particularly when building with more recent JDK versions as that guarantees that you won't also use JDK12+ APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this section from the current core pom.xml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thought behind always enabling the build for the eclipse plugin was to make breakages more apparent (i.e. it ensures that breakages along the way are not just discovered when trying to build the next release version). But, again, if you don't like it, it can be dropped form the PR.
eclipse_plugin/pom.xml
Outdated
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-compiler-plugin</artifactId> | ||
<configuration> | ||
<source>11</source> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this section from the current core pom.xml
.
Added logic to skip maven javadoc build for google java formatter Eclipse plugin. |
True, I would:
|
Given that there recently was another release without an Eclipse plugin, it is really cool to see some improvements being worked on for the plugin build. As a user of google-java-format, I would like to say thanks! Is there a chance that we get an official build of the plugin with google-java-format 1.8 after this gets merged? |
Reworked the PR and updated the PR message to match the new content.
I force-pushed the changes to clean up the commits of this PR. I hope this is not an issue. The new build logic now works independently of the core build. The dependencies are pulled from maven instead of being provided by the core build. Local core snapshots can also be used for the build. The change to always also build the Eclipse plugin as part of the core build has also been reverted as you did not seem to like it. This, however, also means that the new build logic is not covered by the CI. |
@plumpy Are there issues with the PR? Is there something I can do to help it get reviewed? I would really appreciate any feedback. |
The build works fine for me. Using the plugin on Eclipse 2020-03 without problems. |
It's been a while since anything happened on this PR. I still don't see any open issues with it but haven't gotten any further feedback. As far as I can tell, there also aren't any merge/compatibility issues caused by other changes that were merged in the meantime. Is there a fundamental issue with the PR? The way I understand it from reading the issues regarding the Eclipse plugin, the plugin was not released for newer versions of the formatter as you did not have the time to update the build scripts. But, from the muted response/feedback I have gotten on this PR, it makes me wonder whether you have made the internal decision to drop the Eclipse plugin. This would be sad to here as I was hoping to keep using the google java formatter for both IntelliJ IDEA and Eclipse, but it is still your decision to make. It would, however, be nice to get any response/an official statement on the Eclipse plugin situation to avoid people (pointlessly) investing time providing PRs or reporting bugs for it or asking questions about its status. |
Updates the Eclipse plugin build logic. The logic is now completely separate from the main google java format build. Instead of relying on the build artifacts of the core build, the needed google-java-format and guava build artifacts used for the build are now pulled from maven. Updates google-java-format version used for the build to '1.8', the build JDK to 11, and removes old dependencies which are no longer used. Updates tycho version to 1.7.0 in order to build with JDK > 8. Updates the build guide in the README. Co-authored-by: Kelvin Glaß <[email protected]> Co-authored-by: Tobias Bouschen <[email protected]>
Removes the remnants of the old Eclipse plugin build logic from the core pom as they are no longer needed. Co-authored-by: Kelvin Glaß <[email protected]> Co-authored-by: Tobias Bouschen <[email protected]>
Moves the Google copyright header below the XML version declaration in the Eclipse plugin.xml. Otherwise, reading the XML file fails with newer Java versions (e.g. Java 14).
Rebased onto current master and moved build to use GJF 1.9. |
Please anyone at Google (@tbroyer?), can you provide at least some statement about an intent to consider or not consider this PR? It would be good to know if this path has to be considered hopeless for us users of Eclipse, and time invested to search for another solution for formatting code systematically, or if we can keep some hope that this PR will be merged in the foreseeable future. |
@cushon I see you are the most active... You have no idea on the merge of this PR? Or could you just give us a feedback on the evolution of the eclipse plugin? |
For those looking for a working plugin for the latest eclipse release, here is the one I built from @tobous's branch. google-java-format-eclipse-plugin-1.9.0.zip Extract the zip file and put the plugin jar inside the |
I tried your fix and it works like a charm. Thank you very much! To the google guys: Please value his work with as little as clicking the merge button... |
Sorry for the late reply, I just joined. Looking at suggestions from @tobous and @sormuras - should we enforce a minimal JDK version that contains the Javadoc fix?
|
I separated the Eclipse plugin build from the main build a while ago. As a result, the javadoc job is no longer inherited from the main job, thereby avoiding the issue. |
For those interested in a solution to let Eclipse format code according to the Google style guide AND let Maven fail the build when the formatting is wrong, see here for a solution based on Checkstyle. Please report issues there if something goes wrong (I have only tested this on my install). I hope it is not considered inappropriate to hijack this thread to suggest a workaround that has nothing to do with the Eclipse plugin to the Google formatting library. I believe that this comment may nonetheless be useful because I suspect that many readers of this page are, as I was, eager to solve the concrete code formatting problem, whether by this specific plugin or not. |
I have just built the latest plugin for eclipse from @tobous's branch and rebased on official repo, google-java-format-eclipse-plugin-1.10.0.jar Put the plugin jar inside the |
Does anybody know, why this isn't merged? |
Thanks for merging it. However, it required some updates for 1.10.0 release. I will make a pull request soon. |
I just submitted a new pull request #635. |
Now that this and #635 is merged, I have uploaded a build of the Eclipse plugin to the releases page: https://github.com/google/google-java-format/releases/tag/v1.11.0 Sorry for the silence here. We're not using Eclipse as much these days, and I have limited expertise to support the plugin, but I will try to make sure that PRs to keep it working are merged in a more timely manner. |
Updates the Eclipse plugin build logic. The logic is now completely separate from the main google java format build. Instead of relying on the build artifacts of the core build, the needed google-java-format and guava build artifacts used for the build are now pulled from maven.
Updates google-java-format version used for the build to '1.9', the build JDK to 11, and removes old dependencies which are no longer used.
Updates tycho version to 1.7.0 in order to build with JDK > 8.
Updates the build guide in the README.