-
Notifications
You must be signed in to change notification settings - Fork 145
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
Consider migrating away from guava library (or make its use internal) #771
Comments
Which guava dep are you referring to. It is needed by lsp4j.generator only afaik |
any update here @laeubi |
@cdietrich see the linked issues where it is a concern that lsp4j will pull in guava, so if you say
Is this an optional feature? I'm not very familiar with grade but looking here: it seems to depend in the generator. |
its an optional plugin. the dependencies are compile time only and we make sure there are non runtime deps (see architecture tests) |
@merks can you confirm that LSP4J alone does not pull in guava into the simrel? |
we use xtend and xtend needs it. |
Here we go: |
wont happen on Xtext/Xtend side (project currently has 0 person days cappa per year) |
what i dont get: we did all the hoops to get the runtime guava and xtend free and it still is not enhough |
This already has changed things, guava in general is often not required (see TME change) but only there for historic reasons, so if we identify it is only used because of a dependency then yes it requires some "hoops" ... |
It's not trivial to determine what it requires transitively. Probably a target platform in planner mode requiring the bundles you want from a SimRel repository would help answer that. I think only the generator might pull it in, but I think not... |
@merks can we see this in the eierlegende wollmilchsau in the dependencies view? or should we stop providing sdk to simrel and create a runtime feature ? |
Given me a short while and I will create an *.aggr with a *.aggran for analysis. Note that it doesn't really so much matter what features you ship to SimRel but rather what bundles @laeubi wants to consume. Christoph, could you let me know which bundles you need directly? |
i assume lsp4j, lsp4j.jsonrpc, lsp4j.debug, lsp4j.debug.jsonrpc should be the only ones used @merks we know generator pulls guava and xtend/xbase.lib. the question is who pull lsp4j.generator |
so we would need a runtime feature and contribute it instead of the sdk feature to simrel |
I don't know what the goal is but simply not contributing something to SimRel is probably not the goal. I don't see what's to be gained by excluding it. If folks don't want it don't use it. Perhaps @laeubi can clearly state the goal and to which bundles this goal applies. I can reiterate what @cdietrich with respect to guava. It's pretty intensively used in Xtext and hence by the vast downstream consumer base, so that's not likely to be fixed or to be fixable. |
i understood the goal is to use it in platform. but this should not be an issue if platform does not use generator |
Yes, that was my understanding too, which is why I wonder why the discussion is digressing into the feature's content given that the feature is not relevant to the Platform's needs. |
The server side needs lsp4j + json rpc, the client needs lsp4e see: https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/pull/1476/files so whatever this effectively pulls from lsp4j+rpc is relevant for implementing servers and lsp4e (what requires lsp4j anyways) for clients. |
but if you pull these only you wont pull guava so no issue |
Well its not the first time that one has claimed they use guava because of lsp4j what uses it obviously only for the generator so that's seems to be not true... so what does this generator do? Can it be separated in to an own feature so people using the lsp4j feature have to explicitly opt-in? |
Discussing the content of a feature that you don't use seems a digression to me. Clearly lsp4e uses (requires) guava directly while the parts of lsp4j it requires does not; not even transitively. |
this generator is used in the |
So it's a tool while the rest are a runtime. Having a tool in an SDK feature makes sense... |
m2e, and lsp4e and probably others will use the feature, I just use the artifacts from maven directly here to not hit such problems but we all know that some even advocate to not do that and instead using P2 for everything ... |
Given there is no transitive requirement I think
this suggestion then successfully will resolve the issue from lsp4j side 👍 |
You are arguing that there should be a feature that doesn't include the tools because downstream consumers fell compelled to use features, even when that includes things they don't need. Perhaps such consumers need re-education. In any case, that's a different request than stated in the title which is effectively asking the project to re-architect it's tools (so that compulsive feature consumers aren't confused). |
Yes, I think the lsp4e things looks like a major problem... |
No, consumer will include ls4j feature (because there is nothing else), then see it includes guava and start using it "because it is already a dependency". While I think if there is an We have had this issue in the past where lsp4j+lsp4e have required different strict guava versions, so it is not a theoretical issue I think... this seems to be mostly resolved already but was my main concern about lsp4j (core) requiring guava. |
so having |
features are build with plain old tycho |
I'm confused as to why you would even use the LSP4J feature instead of the selected set of bundles you'll need. Some clients might want the debug adapters, other won't it doesn't seem to be reasonable to have one feature per bundle in the lsp4j world. It's a bundle that versioned properly, doesn't have any third party deps, is not marked as singleton and can be shipped by whichever client needs it in their version. Why we need a feature escapes me. |
Yes, I have the same confusion. It seems to me that these days the primary purpose of a feature is for end-user consumption not to simplify someone's target platform authoring and not for someone to include or import an lsp4j feature in their feature. I just get the sense that time would be better invested. But it's not my time, and I'm not the one who will be responsible to maintain yet more features. |
So why (as an enduser) is should ever want to install lsp4j feature, it does not adds anything to the UI nor does it plugs into anything under the hood its just a set of jars installed in my framework that do nothing (from end user perspective) ... So maybe we can delete the feature all together then if no one sees any value in it. |
You'll need the SDK in your TP if you work on protocol extensions based on the same technology stack.
Now I'm wondering what you are really asking for in the ticket. |
LSP4J does not provide a "consumer" feature, only an SDK feature (as it is named today). The SDK feature is designed for people who want an SDK and it is the feature that we distribute LSP4J into SimRel with. I don't think there is anything further to do here. (FWIW I would be more in favour of deleting the feature than adding a second one!) As a comparison, lsp4e doesn't have a feature at all - the feature for lsp4j makes it convenient to contribute LSP4J to SimRel so we can have a single entry in the b3 aggregation file. |
Google Guava is a collection of useful things but there also lies its weakness as this results in big size, and heavy dependencies on specific versions even if not all parts are affected. Also with today modern java many things can be archived with plain java as well.
As it is know to create dependency-hell problems, I'd like to suggest to:
See
for a discussion where the usage of guava currently prevents usage of LSP4J in eclipse-platform while it seems a technology that should be used even more there and currently requires several plugins to ship their own what can then create even more problems of competing versions.
See also:
The text was updated successfully, but these errors were encountered: