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

Add ApplicationArchive.getResolvedDependency() #39483

Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Mar 15, 2024

No description provided.

@Ladicek Ladicek requested review from aloubyansky and mkouba March 15, 2024 14:55
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Mar 15, 2024
@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 15, 2024

Some changes in independent-projects/boostrap are technically breaking, but I take it this is in fact not a public API?

@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 15, 2024

CC @vsevel

Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

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

Thanks @Ladicek

Copy link

quarkus-bot bot commented Mar 15, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit b755b61.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/opentelemetry

io.quarkus.it.opentelemetry.EndUserEnabledTest.baseTest - History

  • AttributesMap{data={net.protocol.name=http, http.scheme=http, http.method=GET, code.namespace=io.quarkus.it.opentelemetry.util.EndUserResource, http.route=/otel/enduser, http.client_ip=127.0.0.1, net.host.port=8081, net.host.name=localhost, user_agent.original=Apache-HttpClient/4.5.14 (Java/17.0.10), http.response_content_length=0, code.function=dummy, http.status_code=200, http.target=/otel/enduser}, capacity=128, totalAddedValues=13} ==> expected: <testUser> but was: <null> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: AttributesMap{data={net.protocol.name=http, http.scheme=http, http.method=GET, code.namespace=io.quarkus.it.opentelemetry.util.EndUserResource, http.route=/otel/enduser, http.client_ip=127.0.0.1, net.host.port=8081, net.host.name=localhost, user_agent.original=Apache-HttpClient/4.5.14 (Java/17.0.10), http.response_content_length=0, code.function=dummy, http.status_code=200, http.target=/otel/enduser}, capacity=128, totalAddedValues=13} ==> expected: <testUser> but was: <null>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1156)
	at io.quarkus.it.opentelemetry.EndUserEnabledTest.evaluateAttributes...

⚙️ JVM Tests - JDK 21

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

io.quarkus.grpc.example.streaming.VertxVirtualThreadTest.testEmpty - History

  • INTERNAL: Half-closed without a request - io.grpc.StatusRuntimeException
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)
	at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:167)
	at io.grpc.testing.integration.TestServiceGrpc$TestServiceBlockingStub.emptyCall(TestServiceGrpc.java:211)
	at io.quarkus.grpc.example.streaming.VirtualThreadTestBase.testEmpty(VirtualThreadTestBase.java:25)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:1018)

@mkouba
Copy link
Contributor

mkouba commented Mar 19, 2024

Some changes in independent-projects/boostrap are technically breaking, but I take it this is in fact not a public API?

I would not replace those methods but introduce new ones and deprecate the old ones.

It's public API in the sense that any extension can use it and there's no clear contract defined...

@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 19, 2024

So there's actually exactly one change that breaks "public" API, this constructor:

-public PathTreeClassPathElement(PathTree pathTree, boolean runtime, ArtifactKey dependencyKey) {
+public PathTreeClassPathElement(PathTree pathTree, boolean runtime, ResolvedDependency resolvedDependency) {

All other changes are in fact compatible due to default methods.

It is unfortunately not straightforward to preserve the old constructor. While it's trivial to obtain an ArtifactKey from ResolvedDependency, it is not possible to do the opposite.

@mkouba
Copy link
Contributor

mkouba commented Mar 19, 2024

So there's actually exactly one change that breaks "public" API, this constructor:

-public PathTreeClassPathElement(PathTree pathTree, boolean runtime, ArtifactKey dependencyKey) {
+public PathTreeClassPathElement(PathTree pathTree, boolean runtime, ResolvedDependency resolvedDependency) {

All other changes are in fact compatible due to default methods.

It is unfortunately not straightforward to preserve the old constructor. While it's trivial to obtain an ArtifactKey from ResolvedDependency, it is not possible to do the opposite.

I see. That's probably acceptable.

@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 19, 2024

To be absolutely precise, there's also one other "breaking" change:

-public ApplicationArchiveImpl(IndexView indexView, OpenPathTree openTree, ArtifactKey artifactKey) {
+public ApplicationArchiveImpl(IndexView indexView, OpenPathTree openTree, ResolvedDependency resolvedDependency) {

But I'm sure everyone agrees that this is not a public API.

@Ladicek Ladicek merged commit c80bfb1 into quarkusio:main Mar 19, 2024
49 checks passed
@Ladicek Ladicek deleted the application-archive-resolved-dependency branch March 19, 2024 14:40
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants