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

Verify JDK 9 classes. #1064

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Verify JDK 9 classes. #1064

merged 1 commit into from
Jan 29, 2024

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 28, 2024

Motivation:
Use Scala-cli to verify the resulting jar can be used in JDK 9, I think this can be a more complete solution than unzip ...find.
refs: #1040

Result:
Can be tested with scala-cli at nightly job, which was verified with https://github.com/apache/incubator-pekko/actions/runs/7684565710/job/20941211960?pr=1064

ping @Roiocam too

@mdedetrich
Copy link
Contributor

As an alternate suggestion, wouldn't it be simpler to just check the header for some specific .class file that we know should be compiled with JDK9?

@He-Pin He-Pin force-pushed the verify branch 2 times, most recently from 0ec41cd to 77b1bc1 Compare January 28, 2024 07:59
@He-Pin
Copy link
Member Author

He-Pin commented Jan 28, 2024

@mdedetrich Do you mean unzip | find?

@He-Pin He-Pin linked an issue Jan 28, 2024 that may be closed by this pull request
@He-Pin
Copy link
Member Author

He-Pin commented Jan 28, 2024

image

@He-Pin He-Pin marked this pull request as ready for review January 28, 2024 08:50
@He-Pin He-Pin added the build sbt or build of the project label Jan 28, 2024
@He-Pin He-Pin added this to the 1.1.0-M1 milestone Jan 28, 2024
@He-Pin He-Pin linked an issue Jan 28, 2024 that may be closed by this pull request
@He-Pin
Copy link
Member Author

He-Pin commented Jan 28, 2024

@pjfanning I only added this to the nightly build, which I think should be enough, if you like, I think we can add a dedicated job to test this, and it was tested with a dedicated job.

@mdedetrich
Copy link
Contributor

@mdedetrich Do you mean unzip | find?

Yup, even an awk ontop just to parse out the bytecode header in the .class file to see if it matches the JDK 9 bytecode version

@He-Pin
Copy link
Member Author

He-Pin commented Jan 28, 2024

@mdedetrich
The difference is that this approach can test whether the final product really works properly, rather than just checking if xxx.class exists using grep. I think this is a relatively advantageous point. In addition, it can expand to test multiple platforms' products, while if you use grep xxx.class in the end, can only determine whether xxx.class exists, right? At least I saw that Dotty uses this approach as well.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 28, 2024

Because I'm going back to my hometown the day after tomorrow, I want to help resolve some blocker issues for 1.1.0-M1. At least I think this solution can solve the issues associated with it, and it's not such a strange solution because there is already a VersionGenerator now.

@mdedetrich
Copy link
Contributor

@mdedetrich The difference is that this approach can test whether the final product really works properly, rather than just checking if xxx.class exists using grep. I think this is a relatively advantageous point. In addition, it can expand to test multiple platforms' products, while if you use grep xxx.class in the end, can only determine whether xxx.class exists, right? At least I saw that Dotty uses this approach as well.

I understand and I will accept either solution. My main reason for suggesting the simpler approach is that our issue is quite black and white, either the JDK 9 classes don't get generated at all or they get generated with the correct JDK version and if they do its completely correct.

Hence I think that checking whether it works is a bit excessive because at that point we are basically checking the scalac compiler is working where as what our historic issue has been is that the misconfiguration of sbt-osgi/multijvm/etc etc means that certain classes don't get compiled/included

@He-Pin
Copy link
Member Author

He-Pin commented Jan 28, 2024

I understand you too. But in another post, you said we still need to support Java 8 for a while. If we eventually migrate to only supporting Java 11, then this can naturally be deleted, as the project is still in incubation. I think we can bear this cost to ensure our products don't become a laughing stock and to ensure supply chain security, right?

@mdedetrich
Copy link
Contributor

I understand you too. But in another post, you said we still need to support Java 8 for a while. If we eventually migrate to only supporting Java 11, then this can naturally be deleted, as the project is still in incubation. I think we can bear this cost to ensure our products don't become a laughing stock and to ensure supply chain security, right?

Sure, as I said I will accept either solution.

I think there may be a more elegant/simpler one we can talk about/do later.

@Roiocam Roiocam mentioned this pull request Jan 28, 2024
32 tasks
| import scala.concurrent.duration.DurationInt
| implicit val system: ActorSystem = ActorSystem.create("test")
| val future = Source(1 to 3).runWith(
| JavaFlowSupport.Sink.asPublisher[Int](fanout = false).mapMaterializedValue { p =>
Copy link
Member

Choose a reason for hiding this comment

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

I have reviewed this for a while, i am kind of understand why this PR does.

A simple grep pipeline connected to unzip results only checks some of the classes, once these class names change or delete, we have to upgrade the CI too.

Another way that parse out the bytecode header, I think it was too complex.

Copy link
Member

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks. I am glad we have a double insurance for these JDK9 check.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 29, 2024

@mdedetrich @pjfanning I have rebased.

Copy link
Contributor

@nvollmar nvollmar left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin He-Pin merged commit ebb9489 into apache:main Jan 29, 2024
18 checks passed
@He-Pin He-Pin deleted the verify branch January 29, 2024 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build sbt or build of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add CI check for the Java 9+ classes 1.1.x pekko snapshots are no longer including the java 9+ classes
5 participants