Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Remove boilerplate from gradle scripts #176

Merged

Conversation

pavolloffay
Copy link
Member

  • Use gradle implicit conventions
  • Use java 8 in tests
  • use java 8 in crossdock

@codecov-io
Copy link

codecov-io commented May 11, 2017

Codecov Report

Merging #176 into master will increase coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #176      +/-   ##
============================================
+ Coverage     80.89%   81.11%   +0.22%     
- Complexity      460      463       +3     
============================================
  Files            77       77              
  Lines          1790     1790              
  Branches        210      210              
============================================
+ Hits           1448     1452       +4     
+ Misses          251      250       -1     
+ Partials         91       88       -3
Impacted Files Coverage Δ Complexity Δ
.../uber/jaeger/samplers/RemoteControlledSampler.java 92.95% <0%> (+1.4%) 19% <0%> (+1%) ⬆️
...jaeger/reporters/protocols/ThriftUdpTransport.java 83.05% <0%> (+5.08%) 16% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c06ea6...c5e807b. Read the comment docs.

@pavolloffay pavolloffay force-pushed the remove-boilerplate-from-gradle branch from 05707fe to fadaf8e Compare May 11, 2017 16:46
@yurishkuro
Copy link
Member

no objection on gradle changes, but why bump tests to 8? It seems prudent to stay on as low version as possible for best compatibility.

@pavolloffay
Copy link
Member Author

I see the reason for shipping main jars with java6. We are not shipping any test-jar, so why not to use java8? There are no consumers of our tests. Only crossdock is consumed but only internally.

Btw. Java SE 7 End of Public Updates is Jul 2015. Currently, the only supported JDK is 8.

jar {
from sourceSets.main.output
from sourceSets.test.output
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed test-classes from jar, as are not used in crossdock image

@yurishkuro
Copy link
Member

We are not shipping any test-jar, so why not to use java8? There are no consumers of our tests.

But why use java8 when we can use java7? The consumers of the tests are the people who use the main artifacts and want assurances that those artifacts have been tested on the respective JRE. And EOL of nor, java7 is still widely used.

Maybe I]m being paranoid, but it seems like switching to java8 has downsides but no upside.

@vprithvi
Copy link
Contributor

have been tested on the respective JRE

This is not true, we only build and test with the travis default, Oracle JDK 7.

The consumers of the tests are the people who use the main artifacts and want assurances that
those artifacts have been tested on the respective JRE.

To do this, we need to setup matrix builds with JDK 6/7/8, and bump down our test source compatibility to Java 6 as well. (I don't like this idea).

but it seems like switching to java8 has downsides but no upside.

The upside is that we can use new language features, I don't see much of the downsides, tbh.

@jpkrohling
Copy link
Collaborator

I'd vote also on bumping to Java 8 without thinking too much about "older" clients. As @pavolloffay mentioned, Java 7 is EOL'ed for almost two years now. People should really, really be using Java 8 if they are minimally concerned about security: using a platform that stopped receiving updates two years ago is just too dangerous ;-)

If you do have a specific client critical enough to keep shipping this library with Java 7 compatibility, then I'd recommend stating a deadline for switching to Java 8. Like, 2 months from now, we will move to Java 8, ready or not.

@vprithvi
Copy link
Contributor

Also, it looks like the travis documentation was incorrect in saying that the default is JDK7; they seem to be running on Java 8 now.

From the build logs,

$ javac -J-Xmx32m -version
javac 1.8.0_111

@yurishkuro
Copy link
Member

@jpkrohling the discussion here was about 1.8 for tests, not for the main code. I don't think it's a good strategy for an OSS project to be overly restrictive with JDK versions, all it'd achieve is turn some people away from using it. There are neither any exceptional features in 1.8 that we absolutely must use in the tracing client, nor do any of our dependencies require upgrading. And there have been explicit requests to support 1.6, and doing so does not hold us back in any way at the moment.

As for using 1.8 in tests, we discussed internally and are ok with it.

@yurishkuro yurishkuro merged commit f955e48 into jaegertracing:master May 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants