-
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
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).
gsmet
left a comment
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.
adr/0009-extension-structure.adoc
Outdated
| * `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.
Can we consider renaming "deployment" to "build"? (same for deployment-api)
I understand the module is about "preparing a deployment", but this is super confusing to new contributors/users.
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.
Backward compatibility could possibly be preserved by falling back to the "deployment" naming, though at a cost. If we care that much about backward compatibility.
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 consider renaming "deployment" to "build"? (same for deployment-api)
I have wanted this since I first layed eyes on the project!!!
Backward compatibility could possible be preserved by falling back to the "deployment" naming, though at a cost. If we care that much about backward compatibility.
We need to care, we can't expect all extension devs to make big changes all at once
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 really support this idea.
|
I want to propose that while we are changing things, should we not consider deployment->buildtime ? |
gsmet
left a comment
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).
|
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:
|
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:
|
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 |
| * The `runtime` module should focus on the internal implementation and runtime logic, delegating public APIs to the `api` module. | ||
| * The `runtime-api` module contains both the public API and the SPI, allowing other extensions to depend on it without pulling in the full extension. | ||
|
|
||
| For extensions requiring tight-coupling, the `runtime` module can still be used to expose public APIs, but this should be avoided when possible. |
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.
Circling a bit back: do the problems that io.quarkus.platform:platform-bom solves (over io.quarkus:platform-bom) still have a practical relevance?
Mean sure, extension A could have a dependency D with version 42.0 and extension B also a dependency on D but with version 66.0, and then we're getting into the version-surprise territory, depending on the build tool - Maven's nearest-first vs Gradle's (default) highest wins vs (all the other build tools).
3d363d3 to
f083a45
Compare
377695a to
7578b22
Compare
First draft (following #47074)
Ping: @gsmet @geoand @dmlloyd