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

improvement: Add flags to release for JDK 8 #4719

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Jan 15, 2025

No description provided.

@kitbellew
Copy link
Collaborator

@tgodzik didn't see you also submitted a fix for this problem, and uploaded my own (#4720). i will abandon mine if yours succeeds :)

@tgodzik tgodzik force-pushed the switch-to-8 branch 2 times, most recently from ffe47fe to a640a8b Compare January 15, 2025 21:07
@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 15, 2025

I think this should work, I checked and the class files produced are version 52. I just had a weird issue with javadoc, where the target option would be forwarded to it, which makes no sense, since javadoc doesn't support the same options. I just set the options for it to nil

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
@tgodzik tgodzik force-pushed the switch-to-8 branch 4 times, most recently from 26db844 to 84fc5a7 Compare January 16, 2025 11:49
@tgodzik tgodzik requested a review from kitbellew January 16, 2025 12:07
@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 16, 2025

I think we should be good now.

@@ -58,9 +58,9 @@ object TestHelpers {
val out1 = x1.formattedCode
val lines2 = x1.formattedCode.count(_ == '\n')
val (duration2, result2) = formatCode(out1)
def saveFormatted(): Unit = Files.writeString(
def saveFormatted(): Unit = Files.write(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method and the one below was added in JDK 11, but wasn't an issue for released artifacts since it's in the tests only

Copy link
Collaborator

Choose a reason for hiding this comment

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

should we instead add new flags to the Compile / so it doesn't affect Test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, maybe it's better to be consistent here? And the changes are quite small.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good

build.sbt Outdated
"-target:8",
"-release:8",
)
case _ => Seq("-target:8", "-release:8")
Copy link
Collaborator

Choose a reason for hiding this comment

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

rather than adding these twice, to each scala version, should we follow the example of unused below and append separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, done. I at some point I thought those might differ

@@ -58,9 +58,9 @@ object TestHelpers {
val out1 = x1.formattedCode
val lines2 = x1.formattedCode.count(_ == '\n')
val (duration2, result2) = formatCode(out1)
def saveFormatted(): Unit = Files.writeString(
def saveFormatted(): Unit = Files.write(
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we instead add new flags to the Compile / so it doesn't affect Test?

).jvmSettings(crossVersion := CrossVersion.disabled, autoScalaLibrary := false)
).jvmSettings(
javacOptions ++= Seq("-source", "8", "-target", "8"),
Compile / doc / javacOptions := Seq(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure you still need this for doc? especially if you used the one above with Compile /?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it still didn't work without it

kitbellew
kitbellew previously approved these changes Jan 16, 2025
@tgodzik tgodzik merged commit 2305a76 into scalameta:main Jan 16, 2025
21 checks passed
@tgodzik tgodzik deleted the switch-to-8 branch January 16, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants