8000 [backport] Make `-target` support JDK 8 through 19 (and deprecate 5 through 7) by stefan-jurco · Pull Request #9916 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

[backport] Make -target support JDK 8 through 19 (and deprecate 5 through 7) #9916

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 19, 2022
Merged

[backport] Make -target support JDK 8 through 19 (and deprecate 5 through 7) #9916

merged 1 commit into from
Feb 19, 2022

Conversation

stefan-jurco
Copy link

Even though scala >= 2.12.15 supports jvm >=8. It is not possible to use it as a target. This adds jvm-11, jvm-17 and jvm-18 alongside jvm-1.8 as supported target values.

@scala-jenkins scala-jenkins added this to the 2.12.16 milestone Feb 1, 2022
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Feb 1, 2022
@SethTisue
Copy link
Member

Is this a backport from 2.13?

@stefan-jurco
Copy link
Author

Just the test TargetTest.scala as the -target resolution is completely changed in 2.13 and I wanted the change to be as minimal as possible.

@SethTisue
Copy link
Member

I agree that it's fine to pretend 9,10,12,13,14,15,16 don't exist.

It seems peculiar to me to require the new "jvm-11" style for the new flags, but then not to also support "jvm-8". If "jvm-8" actually works, could you test that in TargetTests? And if it isn't supported, could you add the support?

Have you verified that the bytecode version actually gets set as expected?

@stefan-jurco stefan-jurco force-pushed the support-target-jvm-ge-8 branch from a6c3e8e to 9e1b0ef Compare February 2, 2022 23:10
@stefan-jurco stefan-jurco changed the title add jvm-11, jvm-17 and jvm-18 as supported -target values [backport] make -target support JVM 8-18 with deprecated warning for 5-7 Feb 2, 2022
@stefan-jurco
Copy link
Author

Backported support for all jvm versions as are in 2.13.x branch with the same alternative formats.
5,6,7 versions are still deprecated.

The bytecode version was not set, I missed that in the code, thanks. And it's verified but just locally as I haven't found test for that.

$ for v in `seq 5 18`; do scalac -deprecation -target:$v Test.scala; file Test.class; done
warning: -target is deprecated: -target:5 is deprecated, forcing use of 8
one warning found
Test.class: compiled Java class data, version 52.0 (Java 1.8)
warning: -target is deprecated: -target:6 is deprecated, forcing use of 8
one warning found
Test.class: compiled Java class data, version 52.0 (Java 1.8)
warning: -target is deprecated: -target:7 is deprecated, forcing use of 8
one warning found
Test.class: compiled Java class data, version 52.0 (Java 1.8)
Test.class: compiled Java class data, version 52.0 (Java 1.8)
Test.class: compiled Java class data, version 53.0 (Java SE 9)
Test.class: compiled Java class data, version 54.0 (Java SE 10)
Test.class: compiled Java class data, version 55.0 (Java SE 11)
Test.class: compiled Java class data, version 56.0 (Java SE 12)
Test.class: compiled Java class data, version 57.0 (Java SE 13)
Test.class: compiled Java class data, version 58.0 (Java SE 14)
Test.class: compiled Java class data, version 59.0
Test.class: compiled Java class data, version 60.0
Test.class: compiled Java class data, version 61.0
Test.class: compiled Java class data, version 62.0

@stefan-jurco stefan-jurco force-pushed the support-target-jvm-ge-8 branch from 9e1b0ef to f2ecf90 Compare February 3, 2022 20:42
@stefan-jurco
Copy link
Author

Hi @SethTisue is there anything else I should update?

SethTisue
SethTisue reviewed Feb 9, 2022
View reviewed changes
Copy link
Member
@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

lgtm... maybe @som-snytt would like to have a look?

som-snytt
som-snytt reviewed Feb 9, 2022
View reviewed changes
project/ScalaOptionParser.scala Outdated
@@ -126,4 +126,5 @@ object ScalaOptionParser {
private def scaladocPathSettingNames = List("-doc-root-content", "-diagrams-dot-path")
private def scaladocMultiStringSettingNames = List("-doc-external-doc")

private val targetSettingNames = (5 to 18).map(_.toString).flatMap(v => v :: s"jvm-1.$v" :: s"jvm-$v" :: s"1.$v" :: Nil).toList
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private val targetSettingNames = (5 to 18).map(_.toString).flatMap(v => v :: s"jvm-1.$v" :: s"jvm-$v" :: s"1.$v" :: Nil).toList
private val targetSettingNames = (5 to 18).flatMap(v => s"$v" :: s"jvm-1.$v" :: s"jvm-$v" :: s"1.$v" :: Nil).toList

Copy link
Author

Choose a reason for hiding this comment

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

updated

@stefan-jurco stefan-jurco force-pushed the support-target-jvm-ge-8 branch from f2ecf90 to 018d404 Compare February 10, 2022 12:45
@stefan-jurco
Copy link
Author

@som-snytt @SethTisue do you want to review it again or could it be merged?

@som-snytt
Copy link
Contributor
som-snytt commented Feb 19, 2022 •
edited
Loading

I was like what's a ChoiceSettingForcedDefault? I still don't know, but at least now I realize this is a backport to 2.12, not to 2.13.

LGTM within an approximation.

(Noting "wants to merge ... into scala:2.12.x" on the github UI is in tiny glowing blue font on my screen which is not a phone. I'm wearing the correct eyeglasses.)

OK, just read the PR on 2.13 #8060

I would have said the preHook was jumping the shark. I sense a peter pan pun. Oh that was an alligator not a shark. Or a crocodile.

I was aware that AbstractSettings went away at some point, and also that those classes are frustrating to work with, as expressed on that PR.

Footnote: I would have gone with just listing the variants, jvm-1.8, 8, etc. and definitely not supported jvm-17. Where did I just see someone's joking indignation at 1.9? ScalaVersion is the type for versions, but it doesn't parse arbitrary prefixes.

@som-snytt som-snytt merged commit 5d9240a into scala:2.12.x Feb 19, 2022
@Philippus Philippus mentioned this pull request Apr 21, 2022
Merged
@SethTisue SethTisue changed the title [backport] make -target support JVM 8-18 with deprecated warning for 5-7 [backport] make -target support JVM 8-19 with deprecated warning for 5-7 Apr 22, 2022
@SethTisue
Copy link
Member

#10000 extends this to JDK 19.

@SethTisue SethTisue mentioned this pull request May 9, 2022
Closed
@SethTisue SethTisue changed the title [backport] make -target support JVM 8-19 with deprecated warning for 5-7 [backport] Make -target support JDK 8 through 19, with deprecation warning for 5 through 7 May 13, 2022
@SethTisue SethTisue changed the title [backport] Make -target support JDK 8 through 19, with deprecation warning for 5 through 7 [backport] Make -target support JDK 8 through 19 (and deprecate 5 through 7) May 13, 2022
@mdedetrich mdedetrich mentioned this pull request Jun 17, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SethTisue SethTisue SethTisue approved these changes

@som-snytt som-snytt som-snytt approved these changes

Assignees
No one assigned
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.12.16
Development

Successfully merging this pull request may close these issues.
5 participants
@stefan-jurco @SethTisue @som-snytt @scala-jenkins @sjurco
0