Skip to content
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

Closed
laeubi opened this issue Nov 5, 2023 · 40 comments
Closed

Comments

@laeubi
Copy link

laeubi commented Nov 5, 2023

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:

  1. replace all usages where possible with standard java
  2. embeds the code that can't be replaced easily in an internal package

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:

@cdietrich
Copy link
Contributor

Which guava dep are you referring to. It is needed by lsp4j.generator only afaik

@cdietrich
Copy link
Contributor

any update here @laeubi

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

@cdietrich see the linked issues where it is a concern that lsp4j will pull in guava, so if you say

It is needed by lsp4j.generator only afaik

Is this an optional feature? I'm not very familiar with grade but looking here:
https://github.com/eclipse-lsp4j/lsp4j/blob/main/org.eclipse.lsp4j/build.gradle

it seems to depend in the generator.

@cdietrich
Copy link
Contributor

cdietrich commented Nov 8, 2023

its an optional plugin.
there is only one feature currently org.eclipse.lsp4j.sdk

the dependencies are compile time only and we make sure there are non runtime deps (see architecture tests)

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

@merks can you confirm that LSP4J alone does not pull in guava into the simrel?
@cdietrich do you know if lsp4j.generator do really need guava for integral parts?

@cdietrich
Copy link
Contributor

we use xtend and xtend needs it.

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

@cdietrich
Copy link
Contributor

wont happen on Xtext/Xtend side (project currently has 0 person days cappa per year)

@cdietrich
Copy link
Contributor

what i dont get: we did all the hoops to get the runtime guava and xtend free and it still is not enhough

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

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" ...

@merks
Copy link

merks commented Nov 8, 2023

@laeubi

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...

@cdietrich
Copy link
Contributor

cdietrich commented Nov 8, 2023

@merks can we see this in the eierlegende wollmilchsau in the dependencies view?
the question is: how requires generator?

or should we stop providing sdk to simrel and create a runtime feature ?

@merks
Copy link

merks commented Nov 8, 2023

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?

@merks
Copy link

merks commented Nov 8, 2023

Here are what the aggregator (p2 planner) pulls with when aggregating all the lsp4j bundles:

image

So yes, the lsp4j generator does pull in guava.

@cdietrich
Copy link
Contributor

cdietrich commented Nov 8, 2023

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

@merks
Copy link

merks commented Nov 8, 2023

Only the feature and the feature is a "root":

image

This analysis does not include what EPP might use.

@cdietrich
Copy link
Contributor

so we would need a runtime feature and contribute it instead of the sdk feature to simrel

@merks
Copy link

merks commented Nov 8, 2023

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.

@cdietrich
Copy link
Contributor

i understood the goal is to use it in platform. but this should not be an issue if platform does not use generator

@merks
Copy link

merks commented Nov 8, 2023

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.

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

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.

@cdietrich
Copy link
Contributor

cdietrich commented Nov 8, 2023

but if you pull these only you wont pull guava so no issue

@merks
Copy link

merks commented Nov 8, 2023

Yes, from an lsp4j point of view there is no issue.

But lps4e seems to drag in everything except the kitchen sink:

image

We often with duplicates of lsp4j because lsp4e releases separately....

Welcome to Pandora's box...

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

Yes, from an lsp4j point of view there is 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?

@merks
Copy link

merks commented Nov 8, 2023

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.

@cdietrich
Copy link
Contributor

cdietrich commented Nov 8, 2023

this generator is used in the *Protocol.xtend and the like files and generates more expressive java out of it.
it is also offered to clients so if they want to create their own json.rpc based protocol similar to lsp4j and lsp4j.debug they can use it.

@merks
Copy link

merks commented Nov 8, 2023

So it's a tool while the rest are a runtime. Having a tool in an SDK feature makes sense...

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

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.

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 ...

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

but if you pull these only you wont pull guava so no issue

Given there is no transitive requirement I think

So it's a tool while the rest are a runtime. Having a tool in an SDK feature makes sense...

this suggestion then successfully will resolve the issue from lsp4j side 👍

@merks
Copy link

merks commented Nov 8, 2023

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).

@merks
Copy link

merks commented Nov 8, 2023

Yes, I think the lsp4e things looks like a major problem...

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

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.

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 lsp4j feature and e.g. lsp4j.tools feature the no one can argue that lsp4j already requires guava so it is fine for them to do so as well.

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.

@cdietrich
Copy link
Contributor

yes we explicitely solved this: #528 / #529

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

so having lsp4j feature and lsp4j.tools feature seems the next logical step, I would propose a PR but have no clue about the gradle build.

@cdietrich
Copy link
Contributor

cdietrich commented Nov 8, 2023

features are build with plain old tycho
https://github.com/eclipse-lsp4j/lsp4j/blob/main/releng/lsp4j-feature/pom.xml

@szarnekow
Copy link

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.

@merks
Copy link

merks commented Nov 8, 2023

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.

@laeubi
Copy link
Author

laeubi commented Nov 8, 2023

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

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.

@szarnekow
Copy link

You'll need the SDK in your TP if you work on protocol extensions based on the same technology stack.

So maybe we can delete the feature all together then if no one sees any value in it.

Now I'm wondering what you are really asking for in the ticket.

@jonahgraham
Copy link
Contributor

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.

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

No branches or pull requests

5 participants