-
Notifications
You must be signed in to change notification settings - Fork 3k
Extension Structure ADR proposal #48688
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
base: main
Are you sure you want to change the base?
Conversation
I will have a look tomorrow |
Yesterday, I drafted that proposal, and I gave some thought about it last evening (yes, I should have considered doing this in the opposite order). I've two questions:
|
Related (on the linting side): quarkiverse/quarkiverse-devops#231 |
IMO yes (every module should have a JPMS name) - we will have to tackle this eventually and it's better to have it done ahead of time. Perhaps our tooling can set this up automatically?
I think |
The In my personal opinion, public API of an extension should be either in the runtime module, under The runtime part of the extension, which is not a public API, should always be in the I see no reason to expose runtime SPI in a different manner than runtime API. They should both be in the same place. If the runtime API has a reason to delimit an SPI, it can do it in all the traditional manners -- we don't need an extra module in the extension just for a runtime SPI. If we need outside world to be able to depend on a runtime API of an extension without depending on the extension as a whole, the full runtime API of the extension should be in a separate module (again, called My $0.017. |
@Ladicek I think it's mostly a naming issue (what is not a naming issue these days 😄). Let me try to clarify how we’re thinking about the separation between runtime, spi, and potential api modules. API vs. SPIIt’s important to distinguish two concepts (as @dmlloyd said). Here is my attempt:
We agree here: API is about usage, SPI is about participation. The Coupling Dimension(Not this is very badly explained in the draft as @geoand mentioned) Where things get more subtle is around coupling: does the application (or consuming extension) require the extension to be present at runtime?
In other words, this separation is about allowing contributions without forcing runtime presence, more architectural than semantic (and that's my whole problem...). Why not call it api?You suggested that the public API should live in a module called If we were trying to reserve api for consumption (i.e., the user-facing surface) and spi for contribution (i.e., extension points), it makes sense to clearly isolate both. The
That's definitely open for debate and suggestions. What about consumed CDI events?That’s, for me, a great case where things blur: the application can consume events produced by an extension without needing that extension to be present. It feels like a runtime API, but from a dependency standpoint, it behaves more like an SPI (you can consume it, even if the source isn’t there). In those cases, having the event types in the spi module makes sense, not because they are technically SPIs, but because we want to avoid runtime coupling. What about?
WDYT? P.S.: Realizing that this comment is almost as long as the initial ADR... |
+1
+1 |
So in my eyes, SPI is part of API. API is what users are supposed to use. Some usages are consumption, some are participation, but it's all API. If you feel like there's a clear line between consumption and participation, I'll respectfully disagree; outside the most simple cases, participation often requires consumption. In other words, an SPI typically looks like interface Participant {
void doSomething(Context ctx);
} where |
@Ladicek We have an example of clean delimitation (like the info extension). However, let's imagine we can't have a clean cut (and the CDI event is an example of this ambiguity). Should we have an |
Yes indeed, that's what I'm advocating for. |
It seems like a split of api(spi)/runtime could be justified if there could be multiple implementations of that API/SPI, i.e. different extensions implementing them. Otherwise, bundling it all in a single runtime module shouldn't be a big deal, should it? |
If we add the classes into the |
I may have missed it, what's the problem with coupling in cases when there is only one impl of a given API? |
@aloubyansky it's mostly what I tried to explained here: The Coupling Dimension(Not this is very badly explained in the draft as @geoand mentioned) Where things get more subtle is around coupling: does the application (or consuming extension) require the extension to be present at runtime?
In other words, this separation is about allowing contributions without forcing runtime presence, more architectural than semantic (and that's my whole problem...). If we put API+SPI in the
|
Thanks. So strong coupling is still an extension developer choice. |
adr/0009-extension-structure.adoc
Outdated
* `io.<quarkus|quarkiverse>.<extension-name>`: Public API. May include subpackages (excluding `spi`). Example: `io.quarkus.cache`. | ||
* `io.<quarkus|quarkiverse>.<extension-name>.runtime`: Internal implementation. Not part of the public API. May include subpackages (excluding dev). | ||
* `io.<quarkus|quarkiverse>.<extension-name>.runtime.graal`: GraalVM substitutions. Not part of the public API. | ||
* `io.<quarkus|quarkiverse>.<extension-name>`: Public API when requiring tight-coupling. May include subpackages (excluding `api`). Example: `io.quarkus.cache`. Note that this is discouraged in favor of the `runtime-api` module. |
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.
Having the API module be io.<quarkus|quarkiverse>.<extension-name>.api
but having a package name of io.<quarkus|quarkiverse>.<extension-name>
is wrong and will cause problems. Even more confusing when the runtime module is io.<quarkus|quarkiverse>.<extension-name>
but in package io.<quarkus|quarkiverse>.<extension-name>.runtime
. The package name and module name must match except for only very special situations.
What if we call the API JPMS module io.<quarkus|quarkiverse>.<extension-name>.api
(if any) and the runtime JPMS module io.<quarkus|quarkiverse>.<extension-name>.runtime
, with package names that match?
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.
The fact that most extension public API are under io.<quarkus|quarkiverse>.<extension-name>
is quite annoying.
In fact, if the extension has a public API in a runtime-api module, I would enforce that the runtime module does not contain io.<quarkus|quarkiverse>.<extension-name>
, so basically, we should never have the case where both io.<quarkus|quarkiverse>.<extension-name>
and io.<quarkus|quarkiverse>.<extension-name>.api
exist in different modules.
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 think it's OK to have extensions which do not match the convention for package names, however the convention/spec should specify a scheme that is correct, else nothing will be correct. So we should recommend that the module should be ...api
and the package should be ...api
(or that the module and package for the API reside in a completely different, non-Quarkus package/module namespace, as long as the packages match the module name).
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 promised to have a look, sorry for the delay, was deep in other subjects (and PTOs!).
adr/0009-extension-structure.adoc
Outdated
Depending on an `runtime-api` module does not transitively include the full extension (i.e., it avoids pulling in runtime or deployment logic). See <<coupling-api-and-spi>> for more details. | ||
* `deployment`: Contains build-time logic, including `BuildSteps`. | ||
This module may define internal build items. | ||
* `deployment-api`: Contains build-time APIs intended to be reused across multiple extensions. |
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.
For me, nothing in the deployment should be API. It's SPI.
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.
If we consider the following definitions:
- API: Types (classes, interfaces) intended to be used by application code or other extensions to consume functionality. Think: RedisClient, annotations, utility methods, etc.
- SPI: Types that allow others to contribute to the extension’s behavior, typically via user implementations, configuration classes (customizer), or extension points. Think: custom resolvers, listeners, info contributor, etc.
and if we look at some of the deployment-spi, they are not spi. Typically, build items used as "phase barrier", or build item that allows you retrieving a build object. Maybe these cases are abuse of the system, but we have tons of things like this.
If we want to say: it's SPI only, it should only contain build items produced by a depending extension and consumed by the host extension.
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.
That's a good breakdown of the possible usages. But I think that things (build items?) which allow other extensions to consume functionality of an extension (as opposed to contribute to it) do fit better under the SPI heading rather than API, so @gsmet might be right here. There is little (nothing?) in deployment-[as]pi that applications can actually consume directly, so there'd be nothing left under "API" in this case.
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.
What @dmlloyd said is exactly what I meant.
Keeping spi for deployment has an additional benefit, in addition to being the right semantic IMO, we won't break all the extension ecosystem.
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.
There is little (nothing?) in deployment-[as]pi that applications can actually consume directly,...
Not applications but other extensions can - a config mapping with ConfigPhase#BUILD_TIME
is IMO a good example of the deployment API...
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.
IMO config should not be public between modules, usually. I'd like to examine some cases where we are doing this to determine whether I'm wrong or whether we should be solving these problems some other way.
Co-authored-by: Guillaume Smet <[email protected]>
@aloubyansky @gsmet @geoand Could you have another look? |
adr/0009-extension-structure.adoc
Outdated
This module may define internal build items. | ||
* `build-spi`: Contains build-time APIs intended to be reused across multiple extensions for integration purposes. | ||
Extensions contributing to or using build items from other extensions should depend on `build-spi` modules. | ||
* `dev`: Contains runtime classes used exclusively in dev mode (e.g., for the Dev UI). This avoids shipping dev-only classes into production artifacts. |
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 forgot, did we consider calling it runtime-dev
? Theoretically, we may want to introduce build-dev
too.
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.
it makes sense to have it named runtime-dev
as we have runtime-api
and not just api
.
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.
We could revisit later if we need a build-dev, do we have an extension using that kind of module?
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 think we need to think about the transition path.
Because some changes might not be worth all the hassle they will generate (I'm thinking of the deployment
-> build
change for instance).
adr/0009-extension-structure.adoc
Outdated
└── codestart/ | ||
---- | ||
|
||
NOTE: `deployment` and `deployment-spi` have been renamed to `build` and `build-spi`. This is a big change, but Maven relocation will mitigate the breakage. |
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 think we need to take a deep breath here and think about all the consequences of this change. It has relatively low value and could require a massive amount of work.
If we actually need relocations (which I'm not sure of), our BOM will be even more gigantic and it will slow down our Maven builds (it's already in the way as it is).
Also, we need to think about how we will support extensions that have not yet been migrated to the new model. This change impacts us but also impacts the whole ecosystem.
It's THE change that will trigger work on EVERY extension out there.
* If the extension name contains a hyphen, it is recommended to replace it with an underscore (e.g., `quarkus-foo-bar` becomes `io.quarkus.foo_bar`). | ||
|
||
==== `runtime` module | ||
* `io.<quarkus|quarkiverse>.<extension-name>.runtime.internal|impl`: Internal implementation. Not part of the public API. |
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.
Should we decide what to do here before finalizing the ADR?
… it will be explored in another ADR
I've reverted the renaming of the |
I think this is probably a mistake. All the concerns that were raised lately had already been addressed earlier. There's no urgency to force people to migrate their extensions. And capitulating to build tool conventions is a very slippery slope, since those can always change over time. Those were the main concerns IIRC and they were addressed. |
But we don't need to do this urgently IMO. We're establishing a new convention, not a new technical mandate. |
I don't have an iron in this fire, but:
|
== Context | ||
|
||
The Quarkus ecosystem includes more than 800 extensions, many of which were developed without a consistent structure. |
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.
Can we have a definition of what an "extension" is? And possibly what's not?
* `runtime-dev`: Contains runtime classes used exclusively in dev mode (e.g., for the Dev UI). This avoids shipping dev-only classes into production artifacts. | ||
* `codestart`: Contains the _Codestart_ templates for the extension. See https://quarkus.io/guides/extension-codestart[Codestart Guide] for more details. | ||
* `cli`: Contains CLI commands related to the extension. This is optional and should be used only if the extension provides specific CLI functionality. | ||
* `codegen`: Contains code generation utilities such as code pre-processors invoked at build time. |
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 maintain a few "extensions" doing codegen, in most cases the pattern used is:
codegen
-> basically standalone (similar to a Maven plugin)runtime
-> often used only to declare transitive dependenciesdeployment
-> sometimes is just the default place that contains the codegen
I'm still unsure where each part of the code is supposed to go, do you have examples of repositories that already migrated(or open PRs) to this new structure?
* `deployment`: Contains build-time logic, including `BuildSteps`. | ||
This module may define internal build items. | ||
* `deployment-spi`: Contains build-time APIs intended to be reused across multiple extensions for integration purposes. | ||
Extensions contributing to or using build items from other extensions should depend on `build-spi` modules. |
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.
Extensions contributing to or using build items from other extensions should depend on `build-spi` modules. | |
Extensions contributing to or using build items from other extensions should depend on `deployment-spi` modules. |
We aren't talking about naming a directory though, we're talking about naming a module. And ruling out module names because one build system claimed one of its components as a directory name - as if they were .com domains in 1995 - is definitely the tail wagging the dog. Everyone agreed that in the absence of Gradle, "build" is a better name than "deployment" for the submodule. So, the module should be called "build". If someone wants to develop an extension using Gradle (which is, as we know, a minority case), they can name the subdirectory something else and the world will continue to turn. |
Dismissing the review simply because the proposal is now materially different from what I approved.
I know people are not going to like me saying the following and I fully expert to be scolded for mentioning it, but here it goes... Changing the directory from |
Scolding incoming. :-) I have a hard time believing that this would be a significant factor one way or the other, honestly. And if it is a factor, I have a hard time believing that it will still be a factor a week, month, or year later. And I think that there's an equal chance that not changing it will be worse for AI. I tend to believe that making AI "understand" our systems is the exact same kind of problem as making extremely naive junior developers understand our systems. Put another way, if we want cogent AI results, I think the number one thing we can do is make sure our docs are cogent, and I believe that means extremely unambiguous grammar and sentence structure, very clear ideas and principles, etc. The second we start making a single coding decision based off of what we think the impact on AI will be, we've lost in such a profound way that I can't even begin to describe it. Principles first! |
Yeah, I can't say I disagree. It's just something that in this day and, we'll likely have to be aware of more and more |
Let me clarify my last changes. The idea was to split the ADR proposal into two parts:
The goals of this second ADR/exploration are:
|
* `runtime-dev`: Contains runtime classes used exclusively in dev mode (e.g., for the Dev UI). This avoids shipping dev-only classes into production artifacts. | ||
* `codestart`: Contains the _Codestart_ templates for the extension. See https://quarkus.io/guides/extension-codestart[Codestart Guide] for more details. | ||
* `cli`: Contains CLI commands related to the extension. This is optional and should be used only if the extension provides specific CLI functionality. | ||
* `codegen`: Contains code generation utilities such as code pre-processors invoked at build time. |
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 arrive late in this review, but I've searched both the other github discussion and this PR's discussion page, and have not found the rationale for splitting codegen into its own dependency?
Is it just a diehard tidying up of separating things, or is there a modularity reason?
Most of my extensions have codegen, so I'm having a hard time thinking of extensions without codegen, but also extensions with only codegen, or a reason why codegen could have different dependencies, or why someone might want to depend on a codegen module.
To me, so far, codegen are very tightly bound to build steps. I do agree that they could be in a standard package separate from build steps, but don't see the point of making an extra module.
Build items, yes, I do agree that they should be split into their own package, and it's good to make a distinction between internal build items and SPI build items. I'm not sure they require a separate module, but I guess it's useful for decoupling and dependencies?
Anyway, I'm wondering if the codegen
split is purely driven by an obsessive requirement to sort things in separate boxes, or if there's an underlying technical reason?
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.
Codegen is for things like Avro/Protobuf. These are command-line tools you need to run before the build to produce source code (.java, not bytecode) that the application code will use. We cannot do that at build time, because you won't reach "build time" (compilation will fail). We cannot use an annotation processor, because it's not annotations or even Java-based.
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.
Compilation of .java
files should be a part of our workflow. The only reason it isn't is concessions to Maven/Gradle IMO...
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.
We cannot use an annotation processor, because it's not annotations or even Java-based.
This is not technically correct, I quickly got a POC together to show that is completely possible to generate code out of an annotation processor without the need of annotating anything.
https://github.com/andreaTP/poc-source-codegen-annot
I stand with @FroMage and believe that removing artificial distinctions would be a plus if that's still open for discussion.
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.
+1 to compilation included in our workflow.
In this case, it's not only about compilation, it's about generating sources by calling other tools. Which arguably should also be a part of our workflow too, today it's implemented in a very different way though.
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.
It's entirely my fault I guess:
Contains code generation utilities such as code pre-processors invoked at build time
This is clear.
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.
We tried to include these in our workflow, the developer experience was plain horrible: you had to delete classes from your code to enable progress.
What's the issue?
Seems like a technical implementation limitation rather than anything else ...
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.
Anyway, looks like that ship has already sailed, no further comments.
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.
The scope of the ADR is already pretty large, and the ADR has already changed significantly since its inception, so IMO we should move discussions about including compilation into a separate discussion
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.
The tangent is my fault, sorry. To bring it back around to Stef's original confusion then: perhaps we should call it sourcegen
. Then if/when we ever roll it into the workflow we can deprecate and remove the concept.
==== `runtime` module | ||
* `io.<quarkus|quarkiverse>.<extension-name>.runtime.internal|impl`: Internal implementation. Not part of the public API. | ||
* `io.<quarkus|quarkiverse>.<extension-name>.runtime.graal`: GraalVM substitutions. Not part of the public API. | ||
* `io.<quarkus|quarkiverse>.<extension-name>`: Public API when requiring tight-coupling. May include subpackages (excluding `api` and `dev` packages). Example: `io.quarkus.cache`. Note that this is discouraged in favor of the `runtime-api` module. |
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 am left a little skeptical about package rules for private vs public:
io.quarkus.foo.runtime.internal|impl
(private, historical)io.quarkus.foo
(public, historical)io.quarkus.foo.api
(public, new)io.quarkus.foo.deployment
(private, historical)io.quarkus.foo.deployment.spi
(public, new)
My problem is that sometimes it's a higher package that is public, sometimes it's a deeper package. This does not seem consistent or intuitive.
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.
First, we recommend using different modules for public and private packages.
Then, io.quarkus.foo.runtime
is more of something we inherit from, but, hopefully, would be replaced by .api
or .impl
(currently, we have a mix of both).
For deployment
, I'm not against adding .impl or .internals.
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.
[Note that I've updated my comment's list because it was wrong]
Oh, so it's …<extention-name>.runtime.internal
or …<extention-name>.impl
? I read it as …<extention-name>.runtime.internal
or …<extention-name>.runtime.impl
🤔
OK. I don't really understand why we would wait though. Changing the policy doesn't mean we must change everything immediately.
OK, this is still wrong AFAICT. If
|
The fact that Gradle chose |
Using the `Automatic-Module-Name` allows extensions to be used as JPMS modules in the future, even if we do not fully adopt JPMS yet. | ||
Also, it avoids split packages, as the `Automatic-Module-Name` must be unique across all modules in the Quarkus ecosystem: | ||
|
||
- Runtime: `io.<quarkus|quarkiverse>.<extension-name>` |
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.
Shouldn't this be something like io.<quarkus|quarkiverse>.<extension-name>.runtime
? Because otherwise io.<quarkus|quarkiverse>.<extension-name>.api
etc. are sub-packages (ok, I know that nothing like this is formalized in Java) of this root and it might confuse people and tools.
First draft (following #47074)
Ping: @gsmet @geoand @dmlloyd