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

Remove usage of Guave #865

Open
vogella opened this issue Nov 1, 2023 · 12 comments
Open

Remove usage of Guave #865

vogella opened this issue Nov 1, 2023 · 12 comments

Comments

@vogella
Copy link

vogella commented Nov 1, 2023

IIRC I saw recently a discussion from @akurtakov that the usage of Guave in lsp4e would be desired. Opening this issue to capture this.

@ptziegler does the same at the moment for WindowBuilder see for example eclipse-windowbuilder/windowbuilder#615

@vogella
Copy link
Author

vogella commented Nov 1, 2023

Is it actually used? https://github.com/search?q=repo%3Aeclipse%2Flsp4e+guava&type=code does not show any usage in Java files.

@akurtakov
Copy link
Contributor

Guava is not in the packagename. You should search for com.google.common .

@vogella
Copy link
Author

vogella commented Nov 1, 2023

Still only 8 usages. Maybe @ptziegler can help here also after his experience with wb?

@ptziegler
Copy link

From what I can tell, only the following part of Guava is used is lsp4e:

  • Chars.contains(<Char Array>, <Char>)
  • Functions.identity()
  • Strings.nullToEmpty(<String>), Strings.isNullOrEmpty(<String>)
  • ThreadFactoryBuilder
  • Ranges & RangeMap

If necessary, the first four could probably be rewritten to not use Guava anymore, but except for Functions.identity() (which should be identical to java.util.function.Function.identity()), I don't think there is a drop-in replacement.

The usage of ranges pretty much requires Guava (or a library with similar functionality). Whether a migration is worth not carrying a 2MB dependency around, I can't answer.
The reason I work on this for WindowBuilder is primarily because:

  • we still use e.g. the Guava Predicates, which have also been introduced with Java 8
  • we use both Guava and Apache Commons to do basically the same thing (create & manage collections)

Both reason don't seem to apply here.

@sebthom
Copy link
Member

sebthom commented Nov 2, 2023

We are using guava in tm4e (CacheBuilder, GsonBuilder, Iterators, Strings, Lists) which is usually used together with lsp4e. I don't see us removing it from tm4e without introducing other new dependencies. So I don't really see the benefit of removing it from lsp4e.

@laeubi
Copy link
Member

laeubi commented Nov 5, 2023

We are using guava in tm4e (CacheBuilder, GsonBuilder, Iterators, Strings, Lists) which is usually used together with lsp4e.

See

by the way m2e uses guava as well for the sake of CacheBuilder it might be good to extract that part into a separate artifact or have an Eclipse plain vanilla replacement for that.

@laeubi
Copy link
Member

laeubi commented Nov 5, 2023

Also see

for a discussion where the usage of guava currently prevents usage of LSP4e 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.

@HannesWell
Copy link
Member

by the way m2e uses guava as well for the sake of CacheBuilder it might be good to extract that part into a separate artifact or have an Eclipse plain vanilla replacement for that.

That's right but I think this can be reworked to use a LinkedHashMap instead.
In fact it was already changed to use only a LinkedHashMap, but the change was reverted because there were regressions. But this was just a quick solution and with more testing it could probably have been solved without Guava.

And if only a more complex cache is necessary than can reasonably be implemented based on standard java, the caffeine library might be worth a consideration. It is similar to guava's caches but much smaller.

@laeubi
Copy link
Member

laeubi commented Nov 5, 2023

The problem with maps is that these are not reentrant safe and that was the problem with LinkedHashMap as far as I remember. One can usually solve this with one indirection class, nevertheless it was just an example that often one does not really need "guava" but only a small portion of it :-)

@laeubi
Copy link
Member

laeubi commented Nov 8, 2023

We are using guava in tm4e (CacheBuilder, GsonBuilder, Iterators, Strings, Lists) which is usually used together with lsp4e.

But it is not a requirement to use tm4e so I still thing it would be beneficial especially as tm4e now has removed guava dependency:

@vogella
Copy link
Author

vogella commented Nov 8, 2023

I don't see us removing it from tm4e without introducing other new dependencies.

That was a fast turnaround. Thanks @sebthom for the removal of Guava in tm4e.

@sebthom
Copy link
Member

sebthom commented Nov 8, 2023

@vogella I thought that Gson is part of guava, but that is not the case. so removing guava was not as complicated as anticipated.

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

6 participants