Skip to content

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

Merged
SethTisue merged 3 commits intoscala:2.12.xfrom
SethTisue:test-jdk20-on-ci
Mar 9, 2023
Merged

[nomerge] Re-STARR onto 2.12.18-M1 and add JDK 20 (early access) to daily CI matrix#10306
SethTisue merged 3 commits intoscala:2.12.xfrom
SethTisue:test-jdk20-on-ci

Conversation

@SethTisue
Copy link
Copy Markdown
Member

@SethTisue SethTisue commented Feb 10, 2023 •
edited
Loading

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
Copy Markdown
Member Author

SethTisue commented Feb 10, 2023 •
edited
Loading

"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
Copy Markdown
Member Author

SethTisue commented Feb 10, 2023 •
edited
Loading

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
Copy Markdown
Member Author

SethTisue commented Mar 4, 2023 •
edited
Loading

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
Copy Markdown
Member Author

SethTisue commented Mar 7, 2023

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
Copy Markdown
Member Author

@SethTisue SethTisue Mar 9, 2023

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
isPublic = true
privateWithin = <none>
============
#partest !java20+
Copy link
Copy Markdown
Member Author

@SethTisue SethTisue Mar 9, 2023 •
edited
Loading

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
Copy Markdown
Member Author

SethTisue commented Mar 9, 2023

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
Test_2.scala:2: error: exception during macro expansion:
java.lang.Exception
at Macros$.impl(Macros_1.scala:6)
#partest java20+
Copy link
Copy Markdown
Contributor

@som-snytt som-snytt Apr 21, 2023

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
Copy Markdown
Member Author

@SethTisue SethTisue Apr 21, 2023

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
Copy Markdown
Contributor

@som-snytt som-snytt Apr 21, 2023

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
Copy Markdown
Member Author

@SethTisue SethTisue Apr 21, 2023

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
Copy Markdown
Contributor

@som-snytt som-snytt Apr 22, 2023

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
Copy Markdown
Member Author

@SethTisue SethTisue Apr 22, 2023 •
edited
Loading

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

Development

Successfully merging this pull request may close these issues.

3 participants

@SethTisue @som-snytt @scala-jenkins