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

Use source file mapping for all compilation providers #46585

Conversation

stuartwdouglas
Copy link
Member

All compilation providers should be setting the source file attribute for use in debugging, there is no need to limit this to Java. This should allow for deletions of Kotlin classes to be handled correctly.

Copy link

quarkus-bot bot commented Mar 2, 2025

Thanks for your pull request!

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.

@stuartwdouglas stuartwdouglas changed the title use source file mapping for all compilation providers Use source file mapping for all compilation providers Mar 2, 2025
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/kotlin-source-file-detection branch from 6fba0b9 to afa4345 Compare March 2, 2025 23:14

This comment has been minimized.

@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/kotlin-source-file-detection branch 2 times, most recently from cc92dcc to cb21c07 Compare March 2, 2025 23:57

This comment has been minimized.

All compilation providers should be setting the source file attribute
for use in debugging, there is no need to limit this to Java. This should
allow for deletions of Kotlin classes to be handled correctly.
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/kotlin-source-file-detection branch from cb21c07 to c61a3e2 Compare March 3, 2025 00:11
Copy link

quarkus-bot bot commented Mar 3, 2025

Status for workflow Quarkus CI

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

✅ 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.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/infinispan-cache/deployment

io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

  • expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:521)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:435)

⚙️ JVM Tests - JDK 21

📦 extensions/hibernate-orm/deployment

io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest.testFieldAccess - History

  • expected: io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest$ContainedEntity@2bac74dc but was: null - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 

expected: io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest$ContainedEntity@2bac74dc
 but was: null
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest$FieldAccessEnhancedDelegate$1.assertValueAndLaziness(PublicFieldAccessAssociationsTest.java:187)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest.doTestFieldAccess(PublicFieldAccessAssociationsTest.java:106)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest.testFieldAccess(PublicFieldAccessAssociationsTest.java:60)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Makes perfect sense, thanks!

@geoand geoand merged commit 95c58a8 into quarkusio:main Mar 3, 2025
58 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.21 - main milestone Mar 3, 2025
@stuartwdouglas
Copy link
Member Author

Any chance we could get this backported? Not sure what the procedure is these days.

@gsmet
Copy link
Member

gsmet commented Mar 3, 2025

If you just want it in 3.19, I added the backport label and will include it in the next 3.19.

Comment on lines +32 to +35
final StringBuilder sourceRelativeDir = new StringBuilder();
sourceRelativeDir.append(classesDir.relativize(classFilePath.getParent()));
sourceRelativeDir.append(File.separator);
sourceRelativeDir.append(sourceFile);
Copy link
Member

Choose a reason for hiding this comment

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

It was preexisting but is there a reason why we are using a StringBuilder here and not the Path API given we already have Paths?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, I just moved the code, I didn't really look at it.

@gsmet gsmet modified the milestones: 3.21 - main, 3.19.2 Mar 4, 2025
@stuartwdouglas
Copy link
Member Author

If you just want it in 3.19, I added the backport label and will include it in the next 3.19.

Thanks!

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