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.

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

[nomerge] Re-STARR onto 2.12.18-M1 and add JDK 20 (early access) to daily CI matrix #10306

Merged
merged 3 commits into from Mar 9, 2023
Merged

[nomerge] Re-STARR onto 2.12.18-M1 and add JDK 20 (early access) to daily CI matrix #10306

merged 3 commits into from Mar 9, 2023

Conversation

SethTisue
Copy link
Member

@SethTisue SethTisue commented Feb 10, 2023 •
edited

we gave 18 and 19 a pass, but I think enough change has accumulated that it's time we tackle this, or at least see what would be involved

@scala-jenkins scala-jenkins added this to the 2.12.18 milestone Feb 10, 2023
@SethTisue SethTisue added the internal not resulting in user-visible changes (build changes, tests, internal cleanups) label Feb 10, 2023
@SethTisue SethTisue self-assigned this Feb 10, 2023
@SethTisue
Copy link
Member Author

SethTisue commented Feb 10, 2023 •
edited

"Unsupported class file major version 64" — that seems puzzling, since ASM 9.4 has

src/main/java/scala/tools/asm/Opcodes.java:  int V20 = 0 << 16 | 64;
src/main/java/scala/tools/asm/util/ASMifier.java:    classVersions.put(Opcodes.V20, "V20");

did we somehow botch #10185 ?

@SethTisue
Copy link
Member Author

SethTisue commented Feb 10, 2023 •
edited

or maybe we need to re-STARR? maybe the reference compiler needs to support JDK 20 before this will pass

I'll try re-STARRing locally and see if it helps

@SethTisue
Copy link
Member Author

SethTisue commented Mar 4, 2023 •
edited

The problem is reproducible locally with enableOptimizer; library/compile — and of course enableOptimizer is included in setupPublishCore, which is what we bootstrap with.

Confirmed that re-STARR-ing locally makes the problem go away, as expected.

@SethTisue SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Mar 4, 2023
@SethTisue SethTisue mentioned this pull request Mar 6, 2023
Closed
8 tasks
@SethTisue SethTisue changed the title add JDK 20 (early access) to daily CI matrix Re-STARR onto 2.12.18-M1 and add JDK 20 (early access) to daily CI matrix Mar 6, 2023
@SethTisue SethTisue mentioned this pull request Mar 6, 2023
Closed
10 tasks
@SethTisue SethTisue changed the title Re-STARR onto 2.12.18-M1 and add JDK 20 (early access) to daily CI matrix [nomerge] Re-STARR onto 2.12.18-M1 and add JDK 20 (early access) to daily CI matrix Mar 6, 2023
@SethTisue SethTisue mentioned this pull request Mar 6, 2023
Closed
@SethTisue
Copy link
Member Author

we're down to a manageable number of failures:

!!  943 - run/t2318.scala                           [non-zero exit code]
!! 1549 - run/t6989                                 [output differs]
!! 1821 - run/t9529                                 [output differs]
!!  943 - run/t2318.scala                           [non-zero exit code]
!!  140 - neg/macro-exception                       [output differs]
!!  145 - neg/macro-invalidret                      [output differs]
!! 10 - jvm/future-spec                           [output differs]
!! 29 - jvm/scala-concurrent-tck.scala            [output differs]

and many of these might just be deprecation warnings

SethTisue
SethTisue commented Mar 9, 2023
View reviewed changes
test/junit/scala/tools/testing/AllocationTest.scala
}
min
}
def min = allocations.min
}
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 wondered if it was written this way in order to avoid allocating, but we have the same change on the 2.13 branch: d79f001

SethTisue
SethTisue commented Mar 9, 2023
View reviewed changes
test/files/run/t6989.check
@@ -107,12 +107,14 @@ isProtected = false
isPublic = true
privateWithin = <none>
============
#partest !java20+
Copy link
Member Author

@SethTisue SethTisue Mar 9, 2023 •
edited

Choose a reason for hiding this comment

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

I wondered if this might be some kind of regression, but on 2.13.x the same change was already made in d943572

@SethTisue SethTisue marked this pull request as ready for review March 9, 2023 00:23
@SethTisue SethTisue force-pushed the test-jdk20-on-ci branch 2 times, most recently from c606dba to 2530369 Compare March 9, 2023 01:42
@SethTisue
Copy link
Member Author

I've removed the "DON'T MERGE ME -- temporarily enable GHA on this PR only" commit now that I've seen green runs.

@SethTisue SethTisue merged commit 28c0d12 into scala:2.12.x Mar 9, 2023
@SethTisue SethTisue deleted the test-jdk20-on-ci branch March 9, 2023 18:46
@SethTisue SethTisue removed the prio:blocker release blocker (used only by core team, only near release time) label Mar 9, 2023
som-snytt
som-snytt reviewed Apr 21, 2023
View reviewed changes
test/files/neg/macro-exception.check
@@ -1,6 +1,12 @@
Test_2.scala:2: error: exception during macro expansion:
java.lang.Exception
at Macros$.impl(Macros_1.scala:6)
#partest java20+
Copy link
Contributor

Choose a reason for hiding this comment

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

could be java19+ so it runs on my machine. The test was previously tweaked to avoid line numbers in stack traces. I think partest still supports filter pragmas that might be good enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

personally I can only get interested in 8, 11, 17, latest; I’ve made a conscious decision not to get finer grained than that without specific motivation

Copy link
Contributor

Choose a reason for hiding this comment

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

In my "other" PR, I added a filter, which is only slightly laborious for me as an enthusiastic contributor.

I've also considered that #partest could be ditched in favor of splitting tests into javaVersion: 17+ etc. Then partest --update-check works seamlessly again.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm. not obvious to me where the other PR is, so I could see what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I almost tagged you. This is the check file I accidentally --updated and overwrote pragmas. 3c69c61

Copy link
Member Author

@SethTisue SethTisue Apr 22, 2023 •
edited

Choose a reason for hiding this comment

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

karma ran over my pragma

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@som-snytt som-snytt som-snytt left review comments

Assignees

@SethTisue SethTisue

Labels
internal not resulting in user-visible changes (build changes, tests, internal cleanups)
Projects
None yet
Milestone
2.12.18
3 participants
@SethTisue @som-snytt @scala-jenkins