Added non-Eclipse libraries as transitive dependencies instead of embed them into the jar #50
Conversation
…xcluded all non-Eclipse libraries from embedding, so they could be excluded by others if there are incompatibility issues
pom.xml
Outdated
| @@ -68,6 +68,93 @@ | |||
| <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding> | |||
| <tychoVersion>2.6.0</tychoVersion> | |||
| </properties> | |||
| <dependencyManagement> | |||
There was a problem hiding this comment.
not sure I fully understand plus many of these are really old. Error prone for example is on 2.11.0 currently (although that one is broken for libraries like mockito) so 2.10.0 is appropriate one. Guava version here is vulnerable last I checked, it needs to be the current one. Closure compiler is 6 years old. JSR305 will do nothing but cause issues if it is even used. It was replaced entirely by spotbugs annotations. Not clear overall why this level of changes are needed. I've never ran into any issue and we actively run this on 100s of repos from jdk 6 to jdk 17 without any issue.
|
I don't see any point here either and only risk. With the 100s of projects that I support that use this, we have never had an issue. There are also 20K or more downloads a month and never saw a reported issue in years. I suspect this is not being used here for the formatter plugin and something else. Need more information here as to the issue. We use the latest guava which is the indicated issue here so it would make no real sense to be an issue in how this was used. The note that its being used with google formatter indicates this is attempted to be used with another project - possibly with spotless plugin but need further clarity why we need to overload how the internals work and then support that without breaking it. Honestly, I would not want to deep dive Eclipse internals to figure out usage of what is there because maven isn't going to tell me and I know we will end up blindly updating dependabot as it won't fail here. I'd suspect the latest release of the module anyways would be on proper guava. We never released 2021-12 yet, 2022-03 isn't even out yet (betas are). So I'd prefer we first move to release 2021-12 and then consume that downstream. This can be left for further information on the issue but suspect that will address the core issue. What we can do is try to make sure we do release this module more frequently. Our wait has been due to massive defects in jsoup. Those are all patched either on jsoup or internally done in formatter. I'm forking jsoup now to release it as-is since I cannot get a respone now from the owner to release as-is and its been months and months of waiting. Personally want to get us out on 2021-12 because we will be immediately following up on 2022-03 in weeks. |
|
I have released 2021-12 now. Will be available shortly. It has guava '30.1-jre' in it which is what this PR was trying to do and what I suspected by pushing update. Still want to understand how this is being used. |
|
adding note: maven build on dependency plugin shows the transients pulled. I still don't feel comfortable about this and think we just release more frequently when Eclipse is finalized every 3 months rather than waiting for formatter plugin to release. That probably will solve the concerns. |
|
Wow, ok, that caused a lot of reaction I wasn't expecting. First of all: I had issues that this library directly including classes of other libraries, in my case that was Guava and so I had no choice than using this version as it is not possible to exclude the Guava version that comes with jsdt-core and as another library (google-java-format) that I use in my project (externalcodeformatter for NetBeans) need a newer version I had a problem. Just releasing another version might solve my issue as well, but in the long run, it might be better to not embed third-party libraries as they can cause a lot of trouble in the classpath. Thanks for all your effort! Cheers, |
Sorry if the reaction seemed intense. Please allow me to explain some of the background for this repo: I guess the confusion from our end is that this jar isn't intended to be used with anything other than the revelc/formatter-maven-plugin. It is merely a repackaging of the upstream org.eclipse jsdt-core jar, because Eclipse isn't publishing that to Maven Central like they are for the jdt-core jar. It was separated into its own repo, to keep the formatter-maven-plugin build simple. But, it's just a repackaging of an upstream jar, in order to serve as a published dependency for our own formatter-maven-plugin. If you need to use jsdt-core as a dependency, I would strongly recommend getting it from Eclipse directly, not from this re-packaging. The current bundling of dependencies works for the purposes it is intended. Yes, it might be possible to change it... but I'm pretty sure the reason it is this way is because we're trying to use the tycho build chain to repackage one of Eclipse's OSGi jars. So, this isn't a standard jar built with normal Maven dependency declarations. Ideally, upstream Eclipse should publish jsdt-core themselves. If they do that, then this repository will no longer be necessary, and if there are any packaging bugs with the jar they eventually end up publishing, you should probably take it up with them. If you really want to use this jar for a purpose other than what it was intended for, I think you could probably repackage the jar, using something like the maven-shade-plugin, and strip out the dependencies you don't want (or relocate them), and then republish that for your own use. However, I probably wouldn't recommend it. It's probably much better to just put pressure on Eclipse to publish their jars to Maven Central. As the README for this repo says, this is intended to be a small project, minimal effort to repackage. So, I don't think we should to be doing any additional build complexity to repackage this dependency to satisfy use cases outside of the formatter-maven-plugin scope. The ultimate goal is for this repo to be unnecessary. We definitely don't want to make it more complicated to maintain. |
|
@funfried no worries thanks for confirming as I figured that was the case. The released copy should now work for you and we should release this bit when eclipse does. This time around we had lots of reasons to hang off as we didn't need this but knowing its being used elsewhere we can do it when eclipse does which should make it more in line. However, one never knows with guava and guava just released a new copy today. |
…c-main # Conflicts: # pom.xml
|
I'm going to close this PR, based on the above conversation, and based on the activity in the forked repository indicating that their intention is to maintain a fork, rather than have it merged back here. |
|
Hello, sorry for not responding, I had the feeling you weren't interested in my changes and the latest version did not work for me either as there is still an older Guava version bundled inside the JAR of your jsdt-core. So I decided to push my changes into a fork. If you would like to have to changes back in your repository, please let me know, I'm happy to file another PR and don't have to maintain the changes myself 😊 Cheers, |
|
@funfried No problem for not responding. I was just documenting the justification for closing this. Given that you have very specific requirements for your use case, I think it makes sense that you maintain a fork. |
|
@funfried I've zapped everything including guava from the delivered package. It only includes eclipse items now. If you could, can you try 3.4.1 and see if that solves your original issues. I'm thinking you could stop publishing the fork if it works out. I only included what seems needed to run the formatter. The file I created is 5mb vs your 12mb. Not sure if that will solve all but would be interested in finding out. I was going to ping you on your repo but you have issues closed there to comment on :) |
|
@funfried Use version 3.4.2 instead. I needed to add org.eclipse.text to the build. |
|
Hi @hazendaz, Thanks a lot for still looking into this, would be great if I could stop publishing my own jsdt-core library! I tried version 3.4.2 but I got runtime issues during my tests: The OSGi stuff seems to be missing now in my case, I guess that's the reason why yours is smaller than mine, I kept the stuff which is needed by jsdt-core, which is maybe not the best option, but I think I had a lot of trouble when I tried to add the OSGi stuff as Maven dependencies in my project. I think I ended up with way too much OSGi inside my NetBeans plugin, but I'm not sure if I remember this correctly, it's 2 years I ago I was investigating this. |
|
Thanks. I'll include that and release again later tonight.
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Fabian Bahle ***@***.***>
Sent: Sunday, April 7, 2024 3:53:57 PM
To: revelc/jsdt-core ***@***.***>
Cc: Jeremy Landis ***@***.***>; Mention ***@***.***>
Subject: Re: [revelc/jsdt-core] Added non-Eclipse libraries as transitive dependencies instead of embed them into the jar (PR #50)
Hi @hazendaz<https://github.com/hazendaz>,
Thanks a lot for still looking into this, would be great if I could stop publishing my own jsdt-core library!
I tried version 3.4.2 but I got runtime issues during my tests:
Errors:
FormatterServiceDelegateTest>NbTestCase.run:275->NbTestCase.runBare:479->NbTestCase.access$200:77->testFormatWithEclipseFormatter:82 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterServiceTest>NbTestCase.run:275->NbTestCase.runBare:479->NbTestCase.access$200:77->testFormat:76 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatSourceLevel_1dot3:252 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatSourceLevel_1dot4:261 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatSourceLevel_1dot5:276 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingEPF:146 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingLinefeed_CR:211 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingLinefeed_CRLF:242 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingLinefeed_LF:226 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingProjectSettings:162 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingProjectSettings_ExplicitDefaultFormatter:181 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingXML:51 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingXML_formatterOnOff:82 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingXML_regionAll:101 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testFormatUsingXML_regions:130 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavaFormatterWrapperTest.testNullFormatterConfig:301 » NoClassDefFound org/osgi/framework/BundleActivator
SpringJavaFormatterServiceTest>NbTestCase.run:275->NbTestCase.runBare:479->NbTestCase.access$200:77->testFormat:78 » NoClassDefFound org/osgi/framework/BundleActivator
SpringJavaFormatterWrapperTest.testFormatUsingLinefeed_CR:143 » NoClassDefFound org/osgi/framework/BundleActivator
SpringJavaFormatterWrapperTest.testFormatUsingLinefeed_CRLF:173 » NoClassDefFound org/osgi/framework/BundleActivator
SpringJavaFormatterWrapperTest.testFormatUsingLinefeed_LF:158 » NoClassDefFound org/osgi/framework/BundleActivator
SpringJavaFormatterWrapperTest.testFormatUsingXML:46 » NoClassDefFound org/osgi/framework/BundleActivator
SpringJavaFormatterWrapperTest.testFormatUsingXML_formatterOnOff:78 » NoClassDefFound org/osgi/framework/BundleActivator
SpringJavaFormatterWrapperTest.testFormatUsingXML_regionAll:97 » NoClassDefFound org/osgi/framework/BundleActivator
SpringJavaFormatterWrapperTest.testFormatUsingXML_regions:127 » NoClassDefFound org/osgi/framework/BundleActivator
EclipseJavascriptFormatterServiceTest>NbTestCase.run:275->NbTestCase.runBare:479->NbTestCase.access$200:77->testFormat:75 » NoClassDefFound org/eclipse/osgi/util/NLS
EclipseJavascriptFormatterWrapperTest.testFormatUsingEPF:85 » NoClassDefFound org/eclipse/osgi/util/NLS
EclipseJavascriptFormatterWrapperTest.testFormatUsingLinefeed_CR:113 » NoClassDefFound Could not initialize class org.eclipse.wst.jsdt.internal.core.util.CommentRecorderParser
EclipseJavascriptFormatterWrapperTest.testFormatUsingLinefeed_CRLF:141 » NoClassDefFound Could not initialize class org.eclipse.wst.jsdt.internal.core.util.CommentRecorderParser
EclipseJavascriptFormatterWrapperTest.testFormatUsingLinefeed_LF:127 » NoClassDefFound Could not initialize class org.eclipse.wst.jsdt.internal.core.util.CommentRecorderParser
EclipseJavascriptFormatterWrapperTest.testFormatUsingProjectSettings:99 » NoClassDefFound Could not initialize class org.eclipse.wst.jsdt.internal.core.util.CommentRecorderParser
EclipseJavascriptFormatterWrapperTest.testFormatUsingXML:45 » NoClassDefFound Could not initialize class org.eclipse.wst.jsdt.internal.core.util.CommentRecorderParser
EclipseJavascriptFormatterWrapperTest.testFormatUsingXML_region:71 » NoClassDefFound Could not initialize class org.eclipse.wst.jsdt.internal.core.util.CommentRecorderParser
EclipseJavascriptFormatterWrapperTest.testFormatUsingXML_regionAll:59 » NoClassDefFound Could not initialize class org.eclipse.wst.jsdt.internal.core.util.CommentRecorderParser
EclipseJavascriptFormatterWrapperTest.testNullFormatterConfig:166 » NoClassDefFound Could not initialize class org.eclipse.wst.jsdt.internal.core.util.CommentRecorderParser
The OSGi stuff seems to be missing now in my case, I guess that's the reason why yours is smaller than mine, I kept the stuff which is needed by jsdt-core, which is maybe not the best option, but I think I had a lot of trouble when I tried to add the OSGi stuff as Maven dependencies in my project. I think I ended up with way too much OSGi inside my NetBeans plugin, but I'm not sure if I remember this correctly, it's 2 years I ago I was investigating this.
—
Reply to this email directly, view it on GitHub<#50 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODI5EX6AJ5EHPFVHIRT3Y4GP5LAVCNFSM5PNXRI3KU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBUGE2TQMZUHAYA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@funfried I've pushed a new version 3.4.4. It includes the eclipse osgi layer. Size is just under 7mb. Can you see if that works or has more issues? |
|
Hi @hazendaz, I tested again with 3.4.4, but unfortunately it still misses some Eclipse libraries: |
|
See #144 For further tracking on this. @funfried Seems we might be at this a while. I'm cool with continuing this process to try to get it where it needs to be. I'll try to get this added shortly. We may end up with the sizing you have which is fine, its till a rather drastic drop from the original which will help CI systems save on space. Is the project you are working on this publicly available so that I might test with it until it works? FWIW - I'm wanting to get the formatter maven plugin out this weekend. In doing so, I may have to just cut the state we end up at and continue that effort separately since our plugin seems fine with what it has. We are also not using OSGI which would explain probably why its not so fragile for us. Anyway, I'll try to address the issue above tomorrow and see where that gets us. Thanks for all your help! |
|
Hi @hazendaz, I'm not using OSGi either, but it seems JSDT needs it during runtime, at least the way I'm using it. My library is in available via Maven Central: https://mvnrepository.com/artifact/de.funfried.libraries/jsdt-core/1.0.7 Thanks again for still looking into this! Would be great if I could drop my library and could just use yours! Cheers, |
As I faced some incompatibility issues with the embedded Google Guava version of jsdt-core and another dependency (google-java-format) where a newer Guava version is needed, I moved all non-Eclipse dependencies out of the jar and added them as transitive dependencies instead, that should make it easier for everyone to exclude some of the dependencies and add other versions instead.