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

Stop removing org.graalvm.polyglot:polyglot artifact from native classpath #37423

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Nov 30, 2023

When #35873 was merged
org.graalvm.polyglot:polyglot was part of the GraalVM dev
builds/releases, but was later completely removed from them.

When quarkusio#35873 was merged
`org.graalvm.polyglot:polyglot` was part of the GraalVM dev
builds/releases, but was later completely removed from them.
Copy link

quarkus-bot bot commented Nov 30, 2023

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@zakkak zakkak requested a review from geoand November 30, 2023 09:48
@zakkak zakkak changed the title org.graalvm.polyglot:polyglot was never shipped with GraalVM Artifact org.graalvm.polyglot:polyglot was never shipped with GraalVM Nov 30, 2023
@zakkak zakkak changed the title Artifact org.graalvm.polyglot:polyglot was never shipped with GraalVM Stop removing org.graalvm.polyglot:polyglot artifact from native classpath Nov 30, 2023
@zakkak
Copy link
Contributor Author

zakkak commented Nov 30, 2023

@jerboaa can you please review this?

@jerboaa
Copy link
Contributor

jerboaa commented Nov 30, 2023

@zakkak Could you please trigger a mandrel/GraalVM CE CI for this? That installs the maven artefacts locally and the quarkus build will use it. I'd like to see this won't cause issues. Thanks!

@jerboaa
Copy link
Contributor

jerboaa commented Nov 30, 2023

Looking at graal-sdk the prime quarkus dep I see polyglot still in the compile deps: https://mvnrepository.com/artifact/org.graalvm.sdk/graal-sdk/23.1.1

So not sure why you said it was never shipped...

@zakkak
Copy link
Contributor Author

zakkak commented Nov 30, 2023

Looking at graal-sdk the prime quarkus dep I see polyglot still in the compile deps: https://mvnrepository.com/artifact/org.graalvm.sdk/graal-sdk/23.1.1

So not sure why you said it was never shipped...

It's no longer shipped as part of GraalVM CE or Mandrel, it's shipped as a maven artifact only. As a result we no longer need to remove it from the classpath (since it won't be double).

@zakkak
Copy link
Contributor Author

zakkak commented Nov 30, 2023

@zakkak Could you please trigger a mandrel/GraalVM CE CI for this? That installs the maven artefacts locally and the quarkus build will use it. I'd like to see this won't cause issues. Thanks!

I scheduled a Mandrel graal/master build with JDK 21/ea in https://github.com/graalvm/mandrel/actions/runs/7045378667

@jerboaa
Copy link
Contributor

jerboaa commented Nov 30, 2023

It's no longer shipped as part of GraalVM CE or Mandrel, it's shipped as a maven artifact only. As a result we no longer need to remove it from the classpath (since it won't be double).

Ah, OK. Got it now. Happy to approve. Please only merge if mandrel CI looks good too.

@zakkak
Copy link
Contributor Author

zakkak commented Nov 30, 2023

I scheduled a Mandrel graal/master build with JDK 21/ea in https://github.com/graalvm/mandrel/actions/runs/7045378667

Well that should have been JDK 22/ea... new workflow https://github.com/graalvm/mandrel/actions/runs/7047225889

@zakkak
Copy link
Contributor Author

zakkak commented Nov 30, 2023

Please only merge if mandrel CI looks good too.

Mandrel CI looks good. Waiting for Quarkus CI.

@zakkak zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 30, 2023
Copy link

quarkus-bot bot commented Nov 30, 2023

Failing Jobs - Building 224e9dd

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 11 🔍
✔️ JVM Tests - JDK 17 🔍
JVM Tests - JDK 21 Build Failures Logs Raw logs 🔍

Full information is available in the Build summary check run.
You can also consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 21 #

- Failing: integration-tests/virtual-threads/grpc-virtual-threads 

📦 integration-tests/virtual-threads/grpc-virtual-threads

io.quarkus.grpc.example.streaming.VertxVirtualThreadTest.testUnary - More details - Source on GitHub

io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request
	at io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:268)
	at io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:249)

@zakkak
Copy link
Contributor Author

zakkak commented Nov 30, 2023

Test failure is irrelevant.

@zakkak zakkak merged commit 8c08a65 into quarkusio:main Nov 30, 2023
51 of 52 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 30, 2023
@zakkak zakkak deleted the 2023-11-30-dont-remove-packages-no-longer-in-graal branch November 30, 2023 21:59
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants