-
Notifications
You must be signed in to change notification settings - Fork 3k
Add devservices runtime module #49025
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 devservices runtime module #49025
Conversation
🎊 PR Preview d035ae3 has been successfully built and deployed to https://quarkus-pr-main-49025-preview.surge.sh/version/main/guides/
|
cdb421d
to
9e7cd87
Compare
ac55bd6
to
dd18e29
Compare
This comment has been minimized.
This comment has been minimized.
dd18e29
to
2fcd309
Compare
Status for workflow
|
Status for workflow
|
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 didn’t do a runtime-dev module because dev services are used by integration and native tests, so I think it needs to be a full runtime module (annoyingly).
Aren't integration (and native) tests are supposed to be LaunchMode.RUN
now?
Oh, possibly! I haven't been following closely. Although I think that makes a |
|
Ohh, I see! That might be worth doing, but it would be a big change, potentially (and possibly one for discussion in #48688). I guess it's sort of allowed by https://github.com/quarkusio/quarkus/pull/48688/files#diff-7b080592c913461229fbbb3fa790210bc36e76d33de9ae965bff9061667fb90eR195, but it might be worth being explicit. |
Yeah, it's not an immediately available alternative at the moment. |
I realised too late that this will mess up quarkiverse extensions; they'll work with 3.27 (if they don't declare a dependency on the runtime module), or 3.28 (if they do), but not both. @aloubyansky, would it would for extensions to mark the dependency optional or would that have all sorts of unforeseen consequences? |
AFAIU, extensions not declaring a dependency on the runtime module will still work in all versions. It's just those that do declare a dependency on the runtime module won't work in versions where this refactoring was not applied? |
Historically, the dev services extension has a deployment module, but no runtime one. Adding a runtime module is non-trivial, because as soon as the runtime module exists, the quarkus extension maven plugin will insist that every extension which uses the deployment module has a dependency on the runtime module. Why not just do the one-time change and add the
runtime
dependency to the poms? That would work for quarkus core repo, but not for the ecosystem. A breaking change like that would be inconvenient for extension authors, but annoying for users, since extensions would only work with 3.26 or 3.25, and not both.Instead, we decided to special-case this module and tolerate missing dependencies. The new module isn't needed for old-style dev services, only new ones. So the goal is that if dev services on the new model are missing the dependency, they should fail with a clear message and early. Ideally, they'd fail at build time, but if not, quickly. Dev services not using the new model should continue to work.
It's not visible in CI, because I fixed it (obviously), but I've confirmed that if I don't have the runtime dependency on a new-style devservice, I get this failure:
I moved some runtime classes over, but not all. I couldn’t move the
crossclassloader
classes over because the StartupImpl needs the registry build item, and the build item needs all the crossclassloader classes. We could switch to reflective access and some interfaces to allow them to move over if we think it’s worth it. That can be a follow-on PR, though, if we do want to do it. It reduces the runtime module size (yay), but adds reflection on the dev service path (boo).I didn’t do a
runtime-dev
module because dev services are used by integration and native tests, so I think it needs to be a full runtime module (annoyingly).So we don't lose track of it, I've raised #49026 as follow-on PR to undo the exemption for Quarkus 4.