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

ArC: fix in-application build compatible extensions #37397

Merged
merged 1 commit into from Nov 30, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Nov 29, 2023

In case the application contains a build compatible extension that registers a synthetic bean for a non-application class, or a synthetic observer declared "as if" in a non-application class, the generated classes must be forced to also be application classes. If the generated classes were non-application classes (as they were prior to this commit), they would not see the creator/disposer/observer classes in Quarkus dev mode and test mode (where application classes are in a different class loader than non-application classes).

Before this commit, it has already been possible to force ArC to generate synthetic bean classes as application classes, but this was not possible for synthetic observers. This commit adds that, too.

Fixes #37337

In case the _application_ contains a build compatible extension that
registers a synthetic bean for a _non-application_ class, or a synthetic
observer declared "as if" in a _non-application_ class, the generated
classes must be forced to also be application classes. If the generated
classes were non-application classes (as they were prior to this commit),
they would not see the creator/disposer/observer classes in Quarkus
dev mode and test mode (where application classes are in a different
class loader than non-application classes).

Before this commit, it has already been possible to force ArC to generate
synthetic bean classes as application classes, but this was not possible
for synthetic observers. This commit adds that, too.
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file labels Nov 29, 2023
</parent>
<modelVersion>4.0.0</modelVersion>

<artifactId>quarkus-arc-test-supplement</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I wonder if this artifact will get released and deployed to the maven central as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. I was thinking I could abuse some other dependency of the ArC deployment module, like AssertJ, but that would require keeping the version string in the test in sync with the POM, which felt like a worse deal than an extra module with a single class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could try to skip this module during a release. I've found https://stackoverflow.com/questions/31495316/skip-submodule-during-maven-release where the following config is used in the module that should be skipped:

<plugin>
  <groupId>org.apache.maven.plugins</groupId>
  <artifactId>maven-deploy-plugin</artifactId>
  <configuration>
    <skip>true</skip>
  </configuration>
</plugin>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would work, but the ArC deployment module has a [test-scoped] dependency on this artifact, so quarkus-arc-deployment POM might cause errors. Is this tiny artifact a problem? I'd much rather keep it than to face surprises down the line.

It would be best if I could create an in-memory archive using ShrinkWrap and add it as a non-application archive, but QuarkusUnitTest doesn't offer an API for that and I don't really know if QuarkusBootstrap.Builder allows it or not. @aloubyansky would you please know if that would be feasible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this tiny artifact a problem?

It is not. The central repo is probably full of other huge useless artifacts anyway 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

QuarkusUnitTest has a nice API for creating application archives using ShrinkWrap, but for this particular test, I need a non-application archive.

Copy link
Member

Choose a reason for hiding this comment

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

I actually haven't tested that. It may fail during dependency resolution due to a missing pom.xml. What I tested was BootstrapAppModelResolver.resolveModel(WorkspaceModule module).

But if anything, we could probably enhance the API to do what we need. It just haven't come up before, afaict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to do something like

ResolvedDependency dep = ResolvedDependencyBuilder.newInstance()
        .setGroupId(groupId)
        .setArtifactId(artifactId)
        .setVersion(version)
        .setResolvedPath(path)
        .setWorkspaceModule(WorkspaceModule.builder()
                .setModuleId(WorkspaceModuleId.of(groupId, artifactId, version))
                .setModuleDir(path)
                .setBuildDir(path)
                .build())
        .build();

(where groupId, artifactId and version are things I made up, and path is the directory where the .class file is), and when I add that dep to forced dependencies, Quarkus tries to download it from Central.

I also tried to add an AdditionalDependency with the forceApplicationArchive flag set to false to the set of additional dependencies, but it doesn't do what I need (probably is still considered an application archive).

Having an extra module in the ArC extension isn't super pretty, but works as I need, so I'll probably leave it be.

Copy link
Member

Choose a reason for hiding this comment

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

It probably fails to resolve the POM artifact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually tries to download both the POM and the JAR:

Downloading from central: https://repo.maven.apache.org/maven2/com/example/test1540098276/1.0.0/test1540098276-1.0.0.pom
Downloading from central: https://repo.maven.apache.org/maven2/com/example/test1540098276/1.0.0/test1540098276-1.0.0.jar

🤷

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 29, 2023

Failing Jobs - Building 36b9976

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

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

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/hibernate-orm/deployment 
! Skipped: extensions/flyway/deployment extensions/hibernate-envers/deployment extensions/hibernate-reactive/deployment and 87 more

📦 extensions/hibernate-orm/deployment

io.quarkus.hibernate.orm.multiplepersistenceunits.MultiplePersistenceUnitsImportSqlHotReloadScriptTest.testImportSqlScriptHotReload - More details - Source on GitHub

io.quarkus.dev.appstate.ApplicationStartException: java.lang.OutOfMemoryError: Metaspace
	at io.quarkus.dev.appstate.ApplicationStateNotification.waitForApplicationStart(ApplicationStateNotification.java:58)
	at io.quarkus.runner.bootstrap.StartupActionImpl.runMainClass(StartupActionImpl.java:132)

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

LGTM

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 30, 2023

MultiplePersistenceUnitsImportSqlHotReloadScriptTest.testImportSqlScriptHotReload is flaky, gonna merge.

@Ladicek Ladicek merged commit 492c80b into quarkusio:main Nov 30, 2023
51 of 52 checks passed
@Ladicek Ladicek deleted the arc-bcextensions-fix branch November 30, 2023 13:18
@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
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file kind/bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoClassDefFoundError in BuildCompatibleExtension
4 participants