-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Support Java 21 main launch protocol #36164
Conversation
P.S. we probably want to add some tests for this, but I am very short on time this week. If I do have find some, I'll add them in a follow up PR |
Interesting thing about this is that we'll support the new styles of writing (We could also easily support |
Yeah, I thought that was funny as well. I'll add the |
|
5534b74
to
1c9d47f
Compare
This comment has been minimized.
This comment has been minimized.
I just realized there's one or two things this PR doesn't do and at least one of them it should. One, the Two, an instance main can be inherited from a superclass. Not sure if we wanna do anything about that, I guess we can just leave that unsupported. There's an interesting interaction between |
Good point!
Ah darn... I do see that https://openjdk.org/jeps/445 mentions a superclass as well... Let me chek what we can do about it.. |
1c9d47f
to
2cb88fd
Compare
@Ladicek WDYT now? I think the superclass case should be handled now |
One thing I am not 100% certain of is the proper order of lookups when there are both |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The JEP says:
That seems quite straightforward to me:
|
Okay, let's try that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would structure the code differently, but it seems OK. The only objection I would have is that there's no test for a combination of multiple valid main
methods (where the correct one must be picked).
Yeah, currentry it does not behave correctly, so I'll need to fix it |
2cb88fd
to
c659e9e
Compare
Should be fixed now (I also added a test to demonstrate it) |
c659e9e
to
ca49710
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@Ladicek ok to merge? |
core/deployment/src/main/java/io/quarkus/deployment/steps/MainClassBuildStep.java
Outdated
Show resolved
Hide resolved
If I'm reading the code correctly, if the main class defined these 2 methods:
We would prefer the 1st, even though we really should prefer the 1st, as specified in https://docs.oracle.com/javase/specs/jls/se21/preview/specs/unnamed-classes-instance-main-methods-jls.html#jls-12.1.4 |
Ah, that's an interesting edge case indeed... |
I'm gonna rethink how this is done |
I would just write an extremely stupid code like this:
|
That's where I am headed - at least unless I find any other problems :) |
This solution is based on transforming the main class so call-sites don't need to change Closes: quarkusio#36154 Alternative to: quarkusio#36158
It was slighly more complicated, but not much. I think that anything other than perhaps extreme edge cases should now be covered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reporting and checking! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of renaming those private
mains to $originalMain$
, that seems fairly risky and I think it would be better to bail out if we detect a collision. But other than that, it seem fine.
I went back and forth on that. Bailing out would definitely have been easier from an implementation standpoint 😎 |
Failing Jobs - Building fd9f134
Full information is available in the Build summary check run. Failures⚙️ Native Tests - Messaging1 #- Failing: integration-tests/reactive-messaging-kafka
📦 integration-tests/reactive-messaging-kafka✖
|
This solution is based on transforming the main class so call-sites don't need to change
Closes: #36154
Alternative to: #36158