-
Notifications
You must be signed in to change notification settings - Fork 460
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
Add support for eclipse 4.13 #482
Conversation
@@ -11,11 +11,10 @@ ext_group=com.diffplug.spotless | |||
ext_VER_JAVA=1.8 | |||
|
|||
# Compile dependencies | |||
VER_ECLIPSE_CDT=9.7 |
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'm wondering if this was a bug in the eclipse-cdt release of 9.8, as it referenced 9.7
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.
Good catch! I looked at it, and I believe you are correct. I should publish a 9.8.1
bugfix.
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.
Thanks for the fix. I really missed to commit the vital change.
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.
Looks like a great start! From here, you are blocked on me publishing these new _ext
jars, I'll let you know when that is complete. I'll push some small changes to this branch as well.
Re: combining build with the main project. The problem is that the builds for these are really slow because of p2, and I don't know of a good way to speed them up. I am open to a build refactor (best to do in a separate PR to prevent blocking this PR), but it needs to not slow down the current build. Everything in _ext
is pretty specialized, it's important for regular java devs to not have to deal with this when contributing normal mavenCentral-based formatters.
_ext/eclipse-base/gradle.properties
Outdated
@@ -1,7 +1,7 @@ | |||
# Mayor versions correspond to the supported Eclipse core version. | |||
# Minor version is incremented for features or incompatible changes (including changes to supported dependency versions). | |||
# Patch version is incremented for backward compatible patches of this library. | |||
ext_version=3.2.1 |
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.
If we don't change the code in eclipse-base
, then we don't need to publish a new eclipse-base
. We only change this when something about the OSGi plumbing needs to change, which is generally rare.
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 was under the impression that the compile-scoped dependencies were included in the fat jar, but if that is incorrect, then I can back this and the related changes out.
Are you OK with me rewriting these commits to suit, or would you prefer new commits?
@@ -11,11 +11,10 @@ ext_group=com.diffplug.spotless | |||
ext_VER_JAVA=1.8 | |||
|
|||
# Compile dependencies | |||
VER_ECLIPSE_CDT=9.7 |
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.
Good catch! I looked at it, and I believe you are correct. I should publish a 9.8.1
bugfix.
@fvgh before I publish this, can you confirm that this is true:
This is because we can just update lockfiles for |
Can you give me push permissions on your PR branch?
If not, can you move your |
Thanks for all your effort. I am really busy at the moment, hence I did not find time to contribute. So if the |
My apologies @nedtwigg , I thought had checked that box, but perhaps you require permissions at the repo level? I granted you maintenance permissions for the Vestmark fork - if that does not work, then I will push this branch to the Diffplug repo, instead. |
#483 opened to capture the build work, will comment with thoughts on that thread. |
|
||
### Version 9.8.1 - October 31st 2019 ([artifact]([jcenter](https://bintray.com/diffplug/opensource/spotless-eclipse-cdt))) | ||
|
||
* Really publish Eclipse CDT release 9.8 for Eclipse 4.12 ([#482](https://github.com/diffplug/spotless/pull/482)). |
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.
:-)
_ext/eclipse-cdt/gradle.properties
Outdated
ext_artifactId=spotless-eclipse-cdt | ||
ext_description=Eclipse's CDT C/C++ formatter bundled for Spotless | ||
ext_description=Eclipse''s CDT C/C++ formatter bundled for Spotless |
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.
not a huge deal, but a typo here, I can fix it but didn't want to mess with your work
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 syntax highlighter in VSCode seemed to think I needed to escape the single-quote. That surprised me, and I think it turns out to be wrong. A fix would be appreciated!
I think we have published everything we need so that you can now do the lockfiles for CDT and for JDT. When those are working, I'll publish the jars for WTP and groovy, and then you can do the lockfiles for those too, and then we can merge and publish! |
JDT & CDT are up, thanks again! |
In the lockfiles, even though eclipse-base can be 3.2.0, we get a bugfix if we ask for 3.2.1. ext-eclipse-groovy 3.5.0 is up and ready for lockfiles. ext-eclipse-wtp 3.15.0 is erroring for me, so it is not up:
|
@nedtwigg - sorry for the compile error - looks like the two artifacts (osgi services and filebuffers) diverged in their versioning, updated accordingly |
@nedtwigg - I have the wtp formatter lock file staged, ready when you are :-) |
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 rest looks good to me. Thanks again for your work.
...tra/src/main/resources/com/diffplug/spotless/extra/groovy_eclipse_formatter/v4.13.0.lockfile
Outdated
Show resolved
Hide resolved
eclipse-wtp 3.15.0 is published (sorry for delay) |
Looks like wtp 3.15.0 has not propagated? |
It claims to be published here: https://bintray.com/diffplug/opensource/spotless-eclipse-wtp# And the mavenCentral sync also claims to be successful. I restart the CI build later today. |
Odd - I was able to run tests locally (assuming travis is running gradlew check, or some derivative thereof), I did not see that failure. |
I reran the build, looks like we're passing CI now! This looks ready to merge to me, except it needs changelog entries for plugin-maven and plugin-gradle. |
changelogs updated, thanks again for all your help @nedtwigg ! |
Released in |
Part 1 of 2: _ext wrapper updates to support 4.13
I broke out (and ordered) the commits by subproject intentionally, although, without project linking / publishing, I have not been able to properly build/test the dependent projects.
I have not attempted to publish, wasn't clear on the process.
Part 2 will come once these libraries have published.
Would you be interested in having the ext projects cleaned up? It would be nice to have project dependencies across these, and potentially the 'root' project.