-
Notifications
You must be signed in to change notification settings - Fork 829
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
Update google java format to 1.9 #1854
Update google java format to 1.9 #1854
Conversation
Ah interesting - since the toolchains only apply to compilations, it doesn't help with google java format since that's actually loaded into Gradle itself. So to do this, we would need to finally deprecate support for building on Java 8 completely. I'd recommend it to avoid getting stuck with old build tooling like we currently are. |
I can't imagine why requiring java 11 for building would be a problem, but it's worth asking if anyone would have a problem with it. @carlosalberto ? |
minor conflict to be resolved on this one, as well. |
fwiw, requiring Java 11 for building will match instrumentation repo |
So I'm a little confused now. In CONTRIBUTING we mention we run under Java 8 (and my tests failing seems we are already broken for Java 8). Maybe it's because I've been out of the loop, but I feel like we increase the required version of Java every month ;) cc @bogdandrutu |
We run tests on Java 8 in CI and they pass fine. We were building on Java 8 on Circle CI and they passed too despite some log messages which you noticed until https://github.com/open-telemetry/opentelemetry-java/pulls?q=is%3Apr+is%3Aclosed which should have fixed that so no log messages. Now we are finally proposing actually requiring modern Java to build. But TBH I'm getting really tired of the continued barriers from the old guard who don't help with this repo at all but only criticize and block the doers and active maintainers. This ownership problem needs to get solved - my recommendation is to remove maintainers that don't actually maintain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think requiring Java 11 for building is totally fine. We can expect devs to have a new Java version available. However, of course (hopefully) we still want to support executing applications using opentelemetry-java with a Java 8 JVM.
This is off topic here. I think this was just a small misunderstanding here. |
+1 to JDK 11 CI/build, but JDK 8 source compatibility |
I would not even care about JDK 8 source compatibility only JDK 8 target compatibility, but unfortunately they are tied together 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we are Java8 source compatible it is fine.
Definitely not locally here. Java 11 runs fine though.
I'm not going to answer here, but please feel fee to raise an actual issue, either in this repo or with the GC ;) To be clear: if we are going to require a newer Java version to build, I'm fine, but this needs to be documented (and as long as Java 8 code can still run against us, etc). |
There might be some extension packages that require Java 11 IIRC. But the core SDK should work with Java 8. Otherwise I think it's a bug. |
Looks like it needs a rebase, @anuraaga . Then, let's do it. BTW, the main branch does currently built for me on java 8, but this one does not, so this will definitely need a doc update, after merging. |
… into google-java-format-19
Thanks added an update to CONTRIBUTING.md Sorry for going off-topic - I read a comment and probably missed the sarcastic intent, but triggered a general feeling of lack of support from some members. But it's no problem, sorry about that :) |
Codecov Report
@@ Coverage Diff @@
## master #1854 +/- ##
=========================================
Coverage ? 85.65%
Complexity ? 1446
=========================================
Files ? 175
Lines ? 5596
Branches ? 581
=========================================
Hits ? 4793
Misses ? 603
Partials ? 200
Continue to review full report at Codecov.
|
We previously couldn't due to building on Java 8 but now we never do so.