eclipse / xtext-lib

Eclipse Xtext™ Libraries
https://www.eclipse.org/Xtext/
Eclipse Public License 2.0
19 stars 33 forks source link

Guava 19+ for org.eclipse.xtext.xbase.lib #30

Closed Dunemaster closed 6 years ago

Dunemaster commented 7 years ago

Bundle org.eclipse.xtext.xbase.lib requires google guava [14-19) Is there any specific reason not to use version 19+?

dhuebner commented 7 years ago

@Dunemaster We are using eclipse orbit p2 repository. There is only 18.0.0 available ATM.

Dunemaster commented 7 years ago

Well, we are using guava 19 in our project and it causes a lot of inconvinience to coax it work with xbase Why not set [14-20] in the manifest?

dhuebner commented 7 years ago

@Dunemaster Because it will cause problems in other environments (e.g. eclipse tycho build) where guava 18 and 19 will be conflicting. Why not use guava 18 in you project?

Dunemaster commented 7 years ago

I am sorry, but what kind of problems can cause allowing (not forcing) other version?

dhuebner commented 7 years ago

e.g. https://bugs.eclipse.org/bugs/show_bug.cgi?id=427862

dhuebner commented 7 years ago

I think this is also related: https://bugs.eclipse.org/bugs/show_bug.cgi?id=482701

Dunemaster commented 7 years ago

Well, currently guava is not a single version dependency, it is a range! I only suggest to broaden it, like it was done in https://bugs.eclipse.org/bugs/show_bug.cgi?id=482701 (https://github.com/eclipse/xtext/commit/14e452fcb1296f3e56bb6696398efac642ffce50)

[10.0.1,14.0.1] -> [14.0,19.0)

I suggest

[14.0,19.0) -> [14.0, 20]

dhuebner commented 7 years ago

@Dunemaster FYI there is an incoming request in eclipse orbit, which will hopefully end up in a new guava contribution 21.0. Means we can consider using guava 21 inclusive upper-bound in eclipse Oxygen -> Xtext 2.12

Dunemaster commented 7 years ago

@dhuebner thank you

pcdavid commented 7 years ago

FYI, Guava 21 is now available in Orbit at http://download.eclipse.org/tools/orbit/S-builds/S20170306214312/repository/ (the repo for Oxygen M6).

cdietrich commented 7 years ago

unfortunately https://github.com/google/guava/wiki/Release21 requires java 8. => we would need to take care we pick guava <=20 for the build but use 21 in eclipse metadata https://github.com/eclipse/xtext-lib/issues/45

cdietrich commented 7 years ago

@svenefftinge @dhuebner what do you think?

guw commented 7 years ago

@cdietrich care must be taken with Guava. You should only spec the range in the metadata that you are comfortable with (read, actually using for compiling). Guava is very good at actually removing deprecated API. For example, in 21 two methods in Objects have been removed which where deprecated in 18. Realistically, you would need to compile with all major versions to make sure that no one uses API that has been introduced, changed and/or removed between 14 and 21.

cdietrich commented 7 years ago

@svenefftinge how shall we proceed with this? update to 21 and skip java 6 support? what should be the new minimal version?

cdietrich commented 7 years ago

https://dev.eclipse.org/mhonarc/lists/cross-project-issues-dev/msg14431.html

cdietrich commented 7 years ago

might need an update of gwt as well (and maybe a different way to build it since the plugin we use seems no longer to be maintained)

cdietrich commented 7 years ago

places that need adaption: {{HiddenRegionFormattingToString.java}} in xtext core uses {{firstNonNull}} we would have to copy & paste the guava code to make this running from 14-21

private static <T> T firstNonNull(T first, T second) {
    return first != null ? first : Preconditions.checkNotNull(second);
}
cdietrich commented 7 years ago

mwe2.lib and mwe2.lanugae.ui still use a restricted guava too.

dhuebner commented 7 years ago

See also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=516799

cdietrich commented 7 years ago

@svenefftinge and i discussed on this issue and we are sceptial if it not too late inside oxygen train to change this critical part

ewillink commented 7 years ago

Mylyn imposes a precisely Guava 21 bound for Oxygen.

It is therefore very desirable, if not mandatory, that Xtext tolerate Guava 21.

Surely you just open the bounds to 21, and write private versions of everything that is lost. This worked for me for OCL and QVTd that depend on Xtext. Not sure how given that Xtext doesn't tolerate Guava 21 (perhaps just any more).

cdietrich commented 7 years ago

that "should" work, but can explode as well. the only private thingy spot I have found was fixed by me but what about the others. and what about non eclipse. and what about mwe ...

svenefftinge commented 7 years ago

Correct me if I'm wrong. I fear that Xtext 2.12 would use guava 21 if it exists (which seem to be likely), even if downstream Xtext languages don't support it. In that case we will have Class collisions between Xtext and Xtext languages.

But that might be true for OCL as well. @ewillink have you tried running it in an Eclipse with multiple guava versions (including 21)?

ewillink commented 7 years ago

MWE, Xtext have demonstrated, perhaps accidentally, that downstream tools can leave Guava bounds unspecified and just inherit whatever choice MWE/Xtext makes. If you're using Xtext, why would you choose anything different?

So long as your Guava use is trivial, e.g. I just use a few Iterables helpers, then migration is trivial.

Obviously if you have Optional in your API it may be a nightmare. The sooner you rewrite the better.

But since Mylyn is Guava 21 only and WONTFIXing its Bugzilla, Xtext really should aim at Guava 21 tolerance.

cdietrich commented 7 years ago

for me this is fewer a problem in xtext code base as with other components (mwe, xcore, ....)

if you want to play "guinea pig" :

http://services.typefox.io/open-source/jenkins///job/xtext-umbrella/job/guava21/lastSuccessfulBuild/artifact/build/p2-repository/

svenefftinge commented 7 years ago

MWE, Xtext have demonstrated, perhaps accidentally, that downstream tools can leave Guava bounds unspecified and just inherit whatever choice MWE/Xtext makes. If you're using Xtext, why would you choose anything different?

I'm thinking of clients that have used previous versions of Xtext and have copied the version range.

But since Mylyn is Guava 21 only and WONTFIXing its Bugzilla, Xtext really should aim at Guava 21 tolerance.

I agree we should do that ASAP. The question is just if this should be done five days before a release. Maybe I am missing something, but since guava is not a singleton bundle it should be ok if mylyn activates and uses a different version. In theory it should only be problematic if the same client uses both Xtext and Mylyn.

ewillink commented 7 years ago

In theory ... However since very few projects have accurate uses directives, it is not clear that Equinox is bug free in selecting a good Guava. If Mylyn forces Guava 21 and Xtext forces Guava 18, then Equinox has a choice wherever there is ambiguity. The problem will show up when one of the big integrators such as Papyrus or Sirius brings Xtext/Mylyn together in one plugin.

I cannot recommend too strongly that Xtext tolerate Guava 21, otherwise we are likely to have a painful time culminating in an Xtext 12.0a at Oxygen RC2. Just the same as in Luna, the final project moved to Guava 15 at RC3.

svenefftinge commented 7 years ago

Ok, we had a brief offline discussion and decided to do it. If it reveals any issues we night roll it back in a bugfix release. @cdietrich agreed to do the changes

ewillink commented 7 years ago

Thanks. wrt clients who have copied a version range, they are ok if they ensure that they only use an earlier Guava version and uninstall Mylyn. If they want Oxygen and Mylyn then they probably need to relax their version bounds. Xtext raising its upperbound shouldn't make anything worse.

ewillink commented 7 years ago

? wrt the commits. Why not just specify the Guava bounds in some mwe.common/xtext-lib plugin and leave all the others to inherit?

cdietrich commented 7 years ago

this is the easy way. for the rest there is again a BIG PILE OF TEST neccessary. i dont want to thing why each of these 20 places has a guava version specified

cdietrich commented 7 years ago

testers wanted: http://services.typefox.io/open-source/jenkins///job/xtext-umbrella/job/master/lastSuccessfulBuild/artifact/build/p2-repository/

spoenemann commented 7 years ago

I'm leaving this open so we can work on updating the Guava Maven dependencies after the release.

cdietrich commented 6 years ago

still blocked by Decide on complete move to Java 8 #51

cdietrich commented 6 years ago

we now use guava 21. thus this is fixed