-
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
Reimplement the extension annotation processor #42141
Conversation
We should have never used the extension processor infra to generate the Maven plugin reference doc. This commit makes sure we don't use anything from the annotation processor.
Also port some of the tests to the new infrastructure.
This should be a lot easier to maintain in the future. Also adapt sync-web-site.sh to the new structure.
We actually have some level of support for building an extension with Gradle so we need to make it work. However, at the moment, it's close to impossible to determine the extension we are in in a Gradle extension. So we allow not detecting the extension and disable the config doc generation in this case.
The annotation processor Filer API doesn't support Windows separators so let's use a plain string.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@gsmet could you please check the docs build failure? |
Ah yes, last minute change... |
I think we need to but it wasn't done before and the website is apparently not ready for it. See how https://quarkus.io/version/main/guides/dev-services misbehaves when all the sections are make searchable.
I'm going to try check myself today but just asking the question here if its something you already considered: is the annotation processor model capturing the "open-ended" properties in a way that tools (IDE) can reason about it? similar for jdbc datasources. I assume it does; but didn't spot it in the example shown. |
We need to make sure people will not confuse them in the future.
@maxandersen I really want to get this in as I want to work on follow ups after that without having the risk of conflicts. We can tweak things later if you have comments. As for your question: yes we have all the information necessary to generate whatever we want for the IDEs, and Maps are supported as they are a key element of our config. A good example of that is what we end up generating for Hibernate ORM Elasticsearch:
The format is not exactly the same but as you can see we have all the information to indicate we have a Map. This was always supported except it was hard to exploit the data. For the IDE format, we will just have to define the format we want and we will generate something they can understand as we have the proper data structures to do it (and they are easy to tweak if something is missing but I think we have all we need). |
Status for workflow
|
cool @gsmet - to be clear I'm not saying no to merge in any form. this As far as I see this doesn't change or introduce new things it cleans up the process and allows (in future) to have config doc generation much saner and scalable. I've wanted this stuff for ages; just looking to spot corner cases. I generally just don't want to put a +1 LGTM on a big PR before having had a chance to run it :) Thus, if you and @aloubyansky are confident about carrying it into the 3.14/3.15 release, go for it. |
Let's say I'm confident enough that the new stuff is a lot more flexible and we will be able to achieve what we want to do. In September, I'll start the discussion with the various tooling teams to define what they are interested in. |
Status for workflow
|
🙈 The PR is closed and the preview is expired. |
It was just that the diff grew from 520ish to 620ish files thus trying to spot what caused the 100 files extra change. If change of includes then makes sense.
/max
…On 7 Aug 2024 at 11:49 +0200, Guillaume Smet ***@***.***>, wrote:
@gsmet commented on this pull request.
In docs/src/main/asciidoc/cache-infinispan-reference.adoc:
> @@ -209,4 +209,4 @@ the cache.
quarkus.cache.infinispan.my-cache.max-idle=100s
----
What do you mean, they weren't there earlier? I'm updating existing includes all over the place? Or can you point me to the place where I add some?
The reason why the naming is different is that the annotation processor is a lot smarter now and the naming used to be derived from the config root name with a lot of weird corner cases.
The naming is now a lot more consistent.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
that was sent 24 hrs ago via email....i guess github had struggles :/ |
Warning
This is a work in progress, do not merge. You can try it at home though.
Fixes #35439
This is the current status for the new extension annotation processor.
If you want to test it, make sure you install it separately first as it looks like Maven doesn't order things as it should. That's something on my TODO list.
I kept the old code as I thought it could still be working but I have some issues I haven't figured out yet. So a full build of the docs will fail in the documentation module for now.
You can fully ignore the
generate_doc
package as it's the old stuff that will be dropped.Main changes
@ConfigMapping
and legacy@ConfigRoot
to avoid having a gazillion of hacks and corner casestarget/
and doesn't respect the boundaries of the module: this prevents us from caching the compiler goal for the extensions. The reason for it is that you can have config for the same extension in the build time and runtime modules, you can have a config group that is in a shared module... which makes it impossible to fully resolved the documentation at the module level. The main idea here is to generate a model per module and then merge the model at the end (in some cases, the merge will be easy, in some others, it's going to be harder - for instance when we were not able to resolve a config group). I'm still experimenting with this part and have several options in mind. That's my next step.Other changes
@ConfigMapping
and legacy@ConfigRoot
in the same extension anymore. It used to not be reliable at all as the detection was quite broken. While I think we could make it work more reliably with the new code, I think we should keep it strict to enforce more checks.Current status
documentation
module doesn't build fine - not sure if it's a simple breakage or if this is inherently broken)@ConfigMapping
and legacy@ConfigRoot
: Core module is using a mix of traditional@ConfigRoot
and@ConfigMapping
#42114 - for these two extensions, I specifically support a mix of both.What's missing
- I need to work on the final assembly of the model: I have two strategies to experiment with - one should be very simple - but is not what I experimented with here, and the other one is more complex and is what I started to implement here (I had the former idea today but not yet sure if it can work).- Once the final assembly is done, we will need to generate the config doc from the module. Atm, I'm thinking that it would actually be good idea to use a JBang Quarkus script and some Qute templating, rather than a Maven plugin. Still a bit unsure as we also need to make sure this is easily usable by extensions in the Quarkiverse and versioned. One thing is for sure, I'd like to use templates as the current generation in plain Java is really hard to follow. @aloubyansky @mkouba maybe you have some thoughts on this?Still some work to do on the Maven plugin generating the AsciiDocHow to experiment
If you want to experiment, here are a few steps:
mvn clean install -Dquickly -f core/processor
extensions/your extension of choice
mvn clean install -DskipTests
runtime
anddeployment
modules, intarget/quarkus-config-doc
, you should have a YAML file containing the resolved model: you can have a look at what you have here, it should be in line with what's currently documented. If not, please point it to me and I will have a look. Note that modules that reference config groups or superclass/interfaces that are outside of the module boundaries will not be fully resolved but for all the simple extensions out there, it should work fine.main
withmvn clean install -Dquickly -f core/processor
Model looks like: