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

Always set JVM 8 target in Scala compiler #1524

Merged
merged 3 commits into from
Oct 29, 2020

Conversation

rshkv
Copy link
Contributor

@rshkv rshkv commented Oct 29, 2020

Before this PR

Projects with gradle-baseline cannot compile Scala code if they set Java targetCompatibility to 11. The error is:

> Task :optimizer-rules:compileScala
'jvm-11' is not a valid choice for '-target'
bad option: '-target:jvm-11'

BaselineScalaStyle sets scalac -target to match that of javac. However scalac 2.12 only accepts target values up-to jvm-8 (see here). Scala version 2.13 supports higher targets, but 2.12 was released July this year and we still need to support it.

After this PR

Edit: No longer set target version for Scala compiler to match that of Java compiler.

==COMMIT_MSG==
Always set target version for Scala compiler to JVM 8.
==COMMIT_MSG==

Possible downsides?

If using scalac 2.13 (which permits jvm-9+), this change can result in a lower target version. I don't think that's dangerous but might be overlooking something.

jvm-7 and lower are disallowed by 2.12 - they wouldn't have been valid targets before. The last 2.11 version was released in 2016.

@changelog-app
Copy link

changelog-app bot commented Oct 29, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

No longer set target version for Scala compiler to match that of Java compiler.

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from CRogers October 29, 2020 00:20
@robert3005
Copy link
Contributor

Scala 2.11 still requires it so either you have to drop it or just set it to 1.8

@rshkv
Copy link
Contributor Author

rshkv commented Oct 29, 2020

True, it defaults to Java 6 in 2.11.12.

@rshkv
Copy link
Contributor Author

rshkv commented Oct 29, 2020

I went with always setting the target to 8 as I don't find a straight-forward way to get the Scala version and offer a target based on that.

Personally, I'd still prefer to not have this here. I don't think we need to support 2.11. And if this happens to fail anyone, they can configure the right target themselves.

@rshkv rshkv changed the title Don't set target JVM for Scala compiler Always set JVM 8 target in Scala compiler Oct 29, 2020
@robert3005
Copy link
Contributor

I think setting it to target 1.8 is correct for now and we can remove it once we are sure we don't need scala 2.11 👍

@rshkv
Copy link
Contributor Author

rshkv commented Oct 29, 2020

@CRogers, the trial-publish job is failing because javadoc is OOMing. I see the same on #1517.

@rshkv
Copy link
Contributor Author

rshkv commented Oct 29, 2020

Also would appreciate a release. This is blocking source compatibility bumps 🙏

@bulldozer-bot bulldozer-bot bot merged commit 9c9b4cb into palantir:develop Oct 29, 2020
@svc-autorelease
Copy link
Collaborator

Failed to load project - please reach out to #dev-foundry-infra or check Aries to debug

@iamdanfox
Copy link
Contributor

I think the changelog is malformed. changelog/@unreleased/pr-1524.yml should be changelog/@unreleased/pr-1524.v2.yml

@rshkv
Copy link
Contributor Author

rshkv commented Oct 29, 2020

Contributors can't edit the changelog-app message.

@rshkv
Copy link
Contributor Author

rshkv commented Oct 29, 2020

Will file PR.

@iamdanfox
Copy link
Contributor

@rshkv you shouldn't have to fork if you're part of the palantir org. then you can edit the msg.

@rshkv
Copy link
Contributor Author

rshkv commented Oct 29, 2020

I'm part of the org but can't push to this repo.

@robert3005
Copy link
Contributor

#1525

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

Successfully merging this pull request may close these issues.

4 participants