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

Upgrades the javac version to 1.8 #474

Closed
wants to merge 1 commit into from
Closed

Conversation

nkibler
Copy link
Contributor

@nkibler nkibler commented Oct 5, 2017

Context: I'm about submit a PR for review that adds some utilities around the libraries.json file that contains metadata on Cloud API libraries. It'd be nice to use some Java 8 features in it (namely Streams).

@codecov
Copy link

codecov bot commented Oct 5, 2017

Codecov Report

Merging #474 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #474   +/-   ##
=======================================
  Coverage   67.05%   67.05%           
=======================================
  Files          75       75           
  Lines        1988     1988           
  Branches      324      324           
=======================================
  Hits         1333     1333           
  Misses        513      513           
  Partials      142      142

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 7b2c75d...74a7409. Read the comment docs.

@loosebazooka
Copy link
Contributor

loosebazooka commented Oct 5, 2017 via email

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

This needs to proceed with caution. Eclipse may be building with 1.7 still.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

This would definitely break CT4E.

@nkibler
Copy link
Contributor Author

nkibler commented Oct 6, 2017

I'll move my code to the IntelliJ plugin, then, since it sounds like Eclipse has its own implementation and won't benefit from it. Meanwhile, I'll leave this PR open because we'll want to upgrade the source eventually anyway. You can merge this in when CT4E migrates to Java 8.

@briandealwis
Copy link
Member

This won't break Eclipse per se, but we're supporting Eclipse Mars (4.5) which still runs on Java 7. I wonder if we have any stats of the JVM versions being used for CT4E…

@nkibler
Copy link
Contributor Author

nkibler commented Oct 6, 2017

I guess I could change this to target 7, but with an 8 source. It should allow for 8 features like Streams, but will likely disallow features that don't have a 7 bytecode analog (like default methods in interfaces). WDYT?

@etanshaul
Copy link
Contributor

etanshaul commented Oct 6, 2017

on second thought isn't that disallowed by the java compiler (source 8 with target 7)?

@loosebazooka
Copy link
Contributor

loosebazooka commented Oct 6, 2017

I think we should at least check if we need to keep supporting mars, given that Eclipse has already released Neon+Oxygen and is on Photon.

Would rather not be involved in java version compiler shenanigans.

@elharo
Copy link
Contributor

elharo commented Oct 10, 2017

There are a lot of old Eclipse versions out in the wild. For comparison, we get GPE bug reports going back at least as far as 4.2. At some point, we will drop Mars support; but I don't want to do that until it buys us something really significant.

@nkibler nkibler closed this Oct 11, 2017
@nkibler nkibler deleted the upgrade-javac-version branch October 11, 2017 17:58
@nkibler
Copy link
Contributor Author

nkibler commented Oct 11, 2017

I'll leave this up to you to do in the future then.

@loosebazooka
Copy link
Contributor

@elharo @chanseokoh @briandealwis can we get some timeline on this?

@elharo
Copy link
Contributor

elharo commented Oct 11, 2017

There's no rush to do this. We can revisit when there's a compelling use case for Java 8 or dropping Eclipse Mars.

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

Successfully merging this pull request may close these issues.

None yet

6 participants