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

ClassfileParser: allow missing param names (for JDK 21) #10397

Merged
merged 1 commit into from May 16, 2023

Conversation

SethTisue
Copy link
Member

@scala-jenkins scala-jenkins added this to the 2.12.18 milestone May 16, 2023
@SethTisue SethTisue added the prio:hi high priority (used only by core team, only near release time) label May 16, 2023
@SethTisue
Copy link
Member Author

SethTisue commented May 16, 2023

my procedure for testing this locally:

  • publish the change locally (with the usual setupPublishCore; publishLocal)
  • sbt -Dsbt.scala.version=2.12.18-bin-SNAPSHOT
  • ++2.12.18-bin-SNAPSHOT!
  • testAll

this gives us, afaics, sufficient confidence to merge the PR. after it's merged, the next steps will be:

  • publish 2.12.18-M2 to Maven Central
  • re-STARR onto 2.12.18-M2, also altering our build to start sbt with sbt -Dsbt.scala.version=2.12.18-M2
  • rebase add JDK 21 Early Access to nightly CI #10394, and if it passes CI as expected, merge it
  • release 2.12.18
  • alter our build again to start sbt with sbt -Dsbt.scala.version=2.12.18
  • wait for @eed3si9n to publish a version of sbt that uses 2.12.18
  • alter our build one last time to start sbt without setting sbt.scala.version

@SethTisue
Copy link
Member Author

SethTisue commented May 16, 2023

on 2.13.x, we'll need to do very similar rigmarole. on that branch, we don't have the circularity between sbt and Scala to contend with, but we will still need to re-STARR in order to be able to bootstrap on JDK 21

we'll merge the fix forward from 2.12.x, also merge forward the sbt -Dsbt.scala.version=2.12.18-M2 change, publish 2.13.11-M2, and re-STARR

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

LGTM

@SethTisue SethTisue marked this pull request as ready for review May 16, 2023 15:44
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label May 16, 2023
@SethTisue SethTisue merged commit c7019c1 into scala:2.12.x May 16, 2023
3 checks passed
@SethTisue SethTisue deleted the bug-12783 branch May 16, 2023 15:45
SethTisue added a commit to SethTisue/dotty that referenced this pull request May 19, 2023
this is a forward port of scala/scala#10397 (fixing scala/bug#12783,
from which Scala 3 also suffers)
smarter added a commit to scala/scala3 that referenced this pull request May 22, 2023
this is a forward port of scala/scala#10397 (fixing scala/bug#12783,
from which Scala 3 also suffers) — the same fix we'll be shipping in
2.12.18 and 2.13.11

I haven't included a unit test, because
* a similar change has already been well validated in a Scala 2 context
* without this change, the compiler can't compile anything at all on JDK
21
* we need the reference compiler to include this change before we can
bootstrap on JDK 21 anyway

but I did manually test it locally, by bootstrapping on JDK 11,
publishing the resulting compiler locally, and then launching the REPL
and evaluating `2 + 2`

on 3.3.0-RC6, that fails with:

> error while loading AccessFlag,
> class file /modules/java.base/java/lang/reflect/AccessFlag.class is
broken, reading aborted with class java.lang.RuntimeException
> bad constant pool index: 0 at pos: 5189
> error while loading ElementType,
> class file /modules/java.base/java/lang/annotation/ElementType.class
is broken, reading aborted with class java.lang.RuntimeException
> bad constant pool index: 0 at pos: 1220
> 2 errors found

whereas with this change, it succeeds.
dongjoon-hyun pushed a commit to apache/spark that referenced this pull request Jun 18, 2023
### What changes were proposed in this pull request?
This PR aims to upgrade Scala to 2.12.18
- https://www.scala-lang.org/news/2.12.18

### Why are the changes needed?
This release adds support for JDK 20 and 21:
- scala/scala#10185
- scala/scala#10362
- scala/scala#10397
- scala/scala#10400

The full release notes as follows:

- https://github.com/scala/scala/releases/tag/v2.12.18

### Does this PR introduce _any_ user-facing change?
Yes, this is a Scala version change.

### How was this patch tested?
Existing Test

Closes #41627 from LuciferYang/SPARK-43832.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
czxm pushed a commit to czxm/spark that referenced this pull request Jun 19, 2023
### What changes were proposed in this pull request?
This PR aims to upgrade Scala to 2.12.18
- https://www.scala-lang.org/news/2.12.18

### Why are the changes needed?
This release adds support for JDK 20 and 21:
- scala/scala#10185
- scala/scala#10362
- scala/scala#10397
- scala/scala#10400

The full release notes as follows:

- https://github.com/scala/scala/releases/tag/v2.12.18

### Does this PR introduce _any_ user-facing change?
Yes, this is a Scala version change.

### How was this patch tested?
Existing Test

Closes apache#41627 from LuciferYang/SPARK-43832.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun pushed a commit to apache/spark that referenced this pull request Jun 19, 2023
### What changes were proposed in this pull request?
This PR aims to upgrade Scala to 2.13.11
- https://www.scala-lang.org/news/2.13.11

Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced by scala/scala#10083, we must fix it when we upgrading to use Scala 3,

### Why are the changes needed?
This release improves collections, adds support for JDK 20 and 21, adds support for JDK 17 `sealed`:
- scala/scala#10363
- scala/scala#10184
- scala/scala#10397
- scala/scala#10348
- scala/scala#10105

There are 2 known issues in this version:

- scala/bug#12800
- scala/bug#12799

For the first one, there is no compilation warning messages related to `match may not be exhaustive` in Spark compile log, and for the second one, there is no case of `method.isAnnotationPresent(Deprecated.class)` in Spark code, there is just

https://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130

in Spark Code, and I checked `javax.annotation.Nonnull` no this issue.

So I think These two issues will not affect Spark itself, but this doesn't mean it won't affect the code written by end users themselves

The full release notes as follows:

- https://github.com/scala/scala/releases/tag/v2.13.11

### Does this PR introduce _any_ user-facing change?
Yes, this is a Scala version change.

### How was this patch tested?
- Existing Test
- Checked Java 8/17 + Scala 2.13.11 using GA, all test passed

Java 8 + Scala 2.13.11: https://github.com/LuciferYang/spark/runs/14337279564
Java 17 + Scala 2.13.11: https://github.com/LuciferYang/spark/runs/14343012195

Closes #41626 from LuciferYang/SPARK-40497.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
czxm pushed a commit to czxm/spark that referenced this pull request Jun 19, 2023
### What changes were proposed in this pull request?
This PR aims to upgrade Scala to 2.13.11
- https://www.scala-lang.org/news/2.13.11

Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced by scala/scala#10083, we must fix it when we upgrading to use Scala 3,

### Why are the changes needed?
This release improves collections, adds support for JDK 20 and 21, adds support for JDK 17 `sealed`:
- scala/scala#10363
- scala/scala#10184
- scala/scala#10397
- scala/scala#10348
- scala/scala#10105

There are 2 known issues in this version:

- scala/bug#12800
- scala/bug#12799

For the first one, there is no compilation warning messages related to `match may not be exhaustive` in Spark compile log, and for the second one, there is no case of `method.isAnnotationPresent(Deprecated.class)` in Spark code, there is just

https://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130

in Spark Code, and I checked `javax.annotation.Nonnull` no this issue.

So I think These two issues will not affect Spark itself, but this doesn't mean it won't affect the code written by end users themselves

The full release notes as follows:

- https://github.com/scala/scala/releases/tag/v2.13.11

### Does this PR introduce _any_ user-facing change?
Yes, this is a Scala version change.

### How was this patch tested?
- Existing Test
- Checked Java 8/17 + Scala 2.13.11 using GA, all test passed

Java 8 + Scala 2.13.11: https://github.com/LuciferYang/spark/runs/14337279564
Java 17 + Scala 2.13.11: https://github.com/LuciferYang/spark/runs/14343012195

Closes apache#41626 from LuciferYang/SPARK-40497.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
G1ng3r pushed a commit to G1ng3r/dotty that referenced this pull request Sep 10, 2023
this is a forward port of scala/scala#10397 (fixing scala/bug#12783,
from which Scala 3 also suffers)
dongjoon-hyun pushed a commit to apache/spark that referenced this pull request Sep 16, 2023
### What changes were proposed in this pull request?
This PR aims to re-upgrade Scala to 2.13.11, after SPARK-45144 was merged, the build issues mentioned in #41943 should no longer exist.

- https://www.scala-lang.org/news/2.13.11

Additionally, this pr adds a new suppression rule for warning message: `Implicit definition should have explicit type`, this is a new compile check introduced by scala/scala#10083, we must fix it when we upgrading to use Scala 3

### Why are the changes needed?
This release improves collections, adds support for JDK 20 and 21, adds support for JDK 17 `sealed`:
- scala/scala#10363
- scala/scala#10184
- scala/scala#10397
- scala/scala#10348
- scala/scala#10105

There are 2 known issues in this version:

- scala/bug#12800
- scala/bug#12799

For the first one, there is no compilation warning messages related to `match may not be exhaustive` in Spark compile log, and for the second one, there is no case of `method.isAnnotationPresent(Deprecated.class)` in Spark code, there is just

https://github.com/apache/spark/blob/8c84d2c9349d7b607db949c2e114df781f23e438/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L130

in Spark Code, and I checked `javax.annotation.Nonnull` no this issue.

So I think These two issues will not affect Spark itself, but this doesn't mean it won't affect the code written by end users themselves

The full release notes as follows:

- https://github.com/scala/scala/releases/tag/v2.13.11

### Does this PR introduce _any_ user-facing change?
Yes, this is a Scala version change.

### How was this patch tested?
- Existing Test

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #42918 from LuciferYang/SPARK-40497-2.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio:hi high priority (used only by core team, only near release time) release-notes worth highlighting in next release notes
Projects
None yet
3 participants