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.

Sign up for GitHub

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

Jump to bottom

TransformerFactory not "used" with GraalVM for JDK 21 (23.1) #35780

Merged
merged 1 commit into from
Sep 11, 2023
Merged

TransformerFactory not "used" with GraalVM for JDK 21 (23.1) #35780

merged 1 commit into from
Sep 11, 2023

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Sep 6, 2023

javax.xml.transform.TransformerFactory does not appear as used in
GraalVM for JDK 21's reports due to inlining.

Closes #35676

@quarkus-bot quarkus-bot bot added area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE labels Sep 6, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 6, 2023

/cc @Sanne (hibernate-orm), @gsmet (hibernate-orm), @yrodiere (hibernate-orm)

@geoand geoand requested a review from Sanne September 6, 2023 12:41
jerboaa
jerboaa reviewed Sep 6, 2023
View reviewed changes
Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Please add a comment as to why we are looking for the new class.

@zakkak
Copy link
Contributor Author

zakkak commented Sep 6, 2023

Please add a comment as to why we are looking for the new class.

Done

jerboaa
jerboaa reviewed Sep 6, 2023
View reviewed changes
...gresql-withxml/src/test/java/io/quarkus/it/jpa/postgresql/JPAFunctionalityInGraalITCase.java Outdated
Comment on lines 24 to 26
// And finally verify we included "com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl" which is
// the fallback implementation class name used in javax.xml.transform.TransformerFactory.newInstance()
// whose invocation gets triggered when io.quarkus.jdbc.postgresql.runtime.graal.SQLXLMFeature is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

... triggered when io.quarkus.jdbc.postgresql.runtime.graal.SQLXLMFeature is enabled.
We cannot use class javax.xml.transform.TransformerFactory directly since delegation to
the implementation might get inlined, thus resulting in 'javax.xml.transform.TransformerFactory'
not showing up as a used class in the reports (due to '-H:+InlineBeforeAnalysis').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Amended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Add a period (.) after SQLXMLFeature is enabled. I.e. ... SQLXMLFeature is enabled.

@zakkak zakkak force-pushed the 2023-09-06-fix-35676 branch 2 times, most recently from c2950e1 to 22de657 Compare September 7, 2023 07:46
jerboaa
jerboaa approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Question: Is the actual working of the XML stuff of the driver actually asserted in the test?

...gresql-withxml/src/test/java/io/quarkus/it/jpa/postgresql/JPAFunctionalityInGraalITCase.java Outdated
Comment on lines 24 to 26
// And finally verify we included "com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl" which is
// the fallback implementation class name used in javax.xml.transform.TransformerFactory.newInstance()
// whose invocation gets triggered when io.quarkus.jdbc.postgresql.runtime.graal.SQLXLMFeature is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Add a period (.) after SQLXMLFeature is enabled. I.e. ... SQLXMLFeature is enabled.

@zakkak
Copy link
Contributor Author

zakkak commented Sep 7, 2023

Seems fine to me. Question: Is the actual working of the XML stuff of the driver actually asserted in the test?

To my understanding yes, in https://github.com/quarkusio/quarkus/blob/main/integration-tests/jpa-postgresql-withxml/src/test/java/io/quarkus/it/jpa/postgresql/JPAFunctionalityTest.java

@zakkak
TransformerFactory not "used" with GraalVM for JDK 21 (23.1)
5d5f6e8 
javax.xml.transform.TransformerFactory does not appear as used in
GraalVM for JDK 21's reports due to inlining.

Closes quarkusio#35676
@jerboaa
Copy link
Contributor

jerboaa commented Sep 7, 2023

Seems fine to me. Question: Is the actual working of the XML stuff of the driver actually asserted in the test?

To my understanding yes, in https://github.com/quarkusio/quarkus/blob/main/integration-tests/jpa-postgresql-withxml/src/test/java/io/quarkus/it/jpa/postgresql/JPAFunctionalityTest.java

Great, thanks!

@zakkak zakkak marked this pull request as ready for review September 8, 2023 06:58
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 8, 2023

✔️ 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.

yrodiere
yrodiere approved these changes Sep 10, 2023
View reviewed changes
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

LGTM, though I'd remove the now unnecessary (and confusing) .assertContainsNot(javax.xml.transform.TransformerFactory.class).
Thanks!

...jpa-postgresql/src/test/java/io/quarkus/it/jpa/postgresql/JPAFunctionalityInGraalITCase.java Show resolved Hide resolved
@yrodiere yrodiere merged commit a296ae1 into quarkusio:main Sep 11, 2023
33 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Sep 11, 2023
@yrodiere
Copy link
Member

Merged, thanks!

@zakkak zakkak deleted the 2023-09-06-fix-35676 branch September 11, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerboaa jerboaa jerboaa approved these changes

@yrodiere yrodiere yrodiere approved these changes

@Sanne Sanne Awaiting requested review from Sanne

Assignees
No one assigned
Labels
area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE kind/bugfix
Projects
None yet
Milestone
3.5.0.CR1
Development

Successfully merging this pull request may close these issues.

GraalVM 24.0/23.1: JPAFunctionalityInGraalITCase.verifyJDKXMLParsersAreIncluded fails
3 participants
@zakkak @jerboaa @yrodiere