-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Introduce Dev only module for relevant extensions #45053
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
Conversation
6300f6b
to
8fd64dc
Compare
/cc @gsmet (hibernate-orm,hibernate-search), @marko-bekhta (hibernate-search), @yrodiere (hibernate-orm,hibernate-search) |
🎊 PR Preview 1c093e3 has been successfully built and deployed to https://quarkus-pr-main-45053-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
The changes seem to look good but we need to check the failures |
This comment has been minimized.
This comment has been minimized.
I'll check why this failed |
We need 8583576 for this CI |
Basically, we need to merge #43618 and rebase |
Signed-off-by: Phillip Kruger <[email protected]>
Signed-off-by: Phillip Kruger <[email protected]>
Signed-off-by: Phillip Kruger <[email protected]>
Signed-off-by: Phillip Kruger <[email protected]>
Signed-off-by: Phillip Kruger <[email protected]>
Signed-off-by: Phillip Kruger <[email protected]>
Signed-off-by: Phillip Kruger <[email protected]>
Signed-off-by: Phillip Kruger <[email protected]>
Status for workflow
|
Status for workflow
|
Alright, the CI looks good. |
@aloubyansky should we merge ? |
Unless @gsmet and @cescoffier had further comments, i'd merge it. |
This is awesome. We can finally say truly that when dev mode does not affect prod jars :) I'm +1 but just a few thing to check/confirm (most likely in follow up prs):
|
3.18.0.CR1
+1 Not sure we reference https://quarkus.io/guides/conditional-extension-dependencies#dev-mode-only-extension-dependencies from anywhere.
It's a feature any extension can use.
It allows enabling certain dependencies only in dev mode. Meaning TEST and PROD builds wouldn't download those and won't include them on the classpath. In this PR though, we are not leveraging this feature to its full potential. Specifically, we keep Dev UI related JARs as deployment dependencies in all modes and use this feature to only promote some of them to the runtime classpath in dev mode. This still avoids leaking a few Dev UI related dependencies into prod and test runtime classpath but they are still dragged along as deployment dependencies and included in We'll explore further reduction in dependency graphs for non-dev modes as the next step. It's doesn't involve only Dev UI, btw.
I don't we have an GH issue for it. The issue is about dependency exclusions not being applied to conditional dependencies in our Gradle plugin implementation. It's not a dev mode specific issue, it's a general issue. Hibernate ORM extension is simply an example of when that happens, which is why it's not included in this refactoring. |
Perhaps, I misunderstood the question. The While this pattern has its purpose and reasons, I am not sure this is something we want to promote as a recommended one. I find it to be complicated, personally. I think a cleaner split (instead of deployment to runtime promotion) would be easier to explain and understand but it has its consequences: basically, extracting Dev UI into its own extension, which is a big conceptual change and we don't want that at this point. What I would like to explore next is actually creating a Dev UI extension for the purpose of reducing the dependency footprint in test and prod modes but leaving the necessary Dev UI SPI as a deployment dependency in all modes to still allow extensions bundle their Dev UI integration (as they do today). |
maybe I'm missing something here - but how could the split be cleaner?
would devui still be default included without user having to add devui extension to their projects ? |
yes so if "runtime-dev" is our internal pattern then this shouldn't be spread to quarkiverse and default templates - but i'm failing to see how this would be different with your suggested cleaner split; should we not at all allow/encourage runtime-dev for quarkiverse when using devmode only dependencies ? |
A clean split would mean removing (most of) the Dev UI classes and resources from the "primary" deployment dependencies by moving them to their own artifacts, possibly extensions and unlinking them from the "primary" extension dependencies. They would only be pulled in in dev mode. The Pushing it further though would lead to creating Dev UI extensions for "primary" extensions, which is a different pattern. I think conceptually, this is what I would prefer but extension developer experience will be more complicated, unless we come up with some simple way to achieve the same effect.
Yea, that's not changing. That's the whole purpose of dev-only dependencies. |
I am not against extensions using this pattern. It works and has its benefits. IMO, it's certainly better than removing Dev UI classes and resources from build steps. I'm just not happy with the complexity of it all and think, perhaps, we should give it some time before we start describing it as "the way" to model Dev UI dependencies. |
This is on top of the work that @aloubyansky did to allow Conditional dependency based on mode (dev mode in this case)
For now I only moved Dev UI Runtime to these new modules.
With some more refactoring we can move other things like HotReload setup and Dev console (I'll try that in another PR)
This PR is fairly big, but there is a commit per extension to make it easier to review. Basically it is the following for each extension:
This means that all bits of dev ui is now removed from a Prod build. Previously the dev ui runtime classes was still part of the final bundle (even though they where not use internally, if users wanted they can create new instances of these classes)