Skip to content
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

Show failing with compiler plugin #273

Closed
wants to merge 1 commit into from
Closed

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Feb 2, 2023

@rbygrave
Copy link
Contributor

rbygrave commented Feb 2, 2023

FYI: blackbox-aspect is missing META-INF/services/io.avaje.inject.spi.Module ...

@SentryMan
Copy link
Collaborator Author

SentryMan commented Feb 2, 2023

it's there in the blackbox-aspect jar created by jar plugin

@rbygrave
Copy link
Contributor

rbygrave commented Feb 3, 2023

it's there in the blackbox-aspect jar created by jar plugin

Ah yeah, I forgot that the annotation processor automatically generates that file for us.

WRT this issue, a "thing that works that we don't want to do" is to add the blackbox-aspect into the <annotationProcessorPaths> <path> like:

          <annotationProcessorPaths>
            <path>
              <groupId>io.avaje</groupId>
              <artifactId>avaje-inject-generator</artifactId>
              <version>${project.version}</version>
            </path>
            <path>
              <groupId>io.avaje</groupId>
              <artifactId>blackbox-aspect</artifactId>
              <version>${project.version}</version>
            </path>
          </annotationProcessorPaths>

... which is a strong indication that it's a classpath / module-path issue.

This makes me ponder that the path going forward might be to replace the service loading with ... find the module classes, find the TypeElement (of the external modules) ... find their methods ... read the @DependencyMeta from the methods. (This is what the AP already does in order to support partial compiles, ... we approximately extend that to read the @DependencyMeta from all the external inject modules as well.

So time for lunch and will ponder that ...

Edit: FYI I added module-info and that didn't fix it and still reproduces the issue so I'm not sure what I did the other night that worked / confused about that at the moment.

@SentryMan
Copy link
Collaborator Author

It seems the classpath just isn't there during the mvn compiler plugin. I tried using Class.forName to try and manually get the module but it didn't work.

@rbygrave
Copy link
Contributor

rbygrave commented Feb 3, 2023

It seems the classpath just isn't there during the mvn compiler plugin

Yes I agree.

This is suggesting to me that ServiceLoader just isn't going to work for us here ... need to instead look at reading the @DependencyMeta via the TypeElements etc. Also means the ServiceLoader for Plugin also isn't going to work.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Feb 3, 2023

It doesn't look like TypeElements works when the classpath isn't there. We may have to do something wacky with the class loading. Or else we can say it's our loss and accept that this will only work when we do it as a provided dependency.

@SentryMan
Copy link
Collaborator Author

jsonb plugin works though, because jsonb is added to the cp by the generator AP

@SentryMan
Copy link
Collaborator Author

SentryMan commented Feb 4, 2023

oh wait, this also means that jsonb imports won't automatically work on dependencies if using the mvn compiler plugin.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Feb 4, 2023

or alternatively, I'm actually panicking for no reason, because type elements do in fact work without the cp. Jsonb is fine.

@rbygrave
Copy link
Contributor

rbygrave commented Feb 4, 2023 via email

@SentryMan
Copy link
Collaborator Author

I have a plan of creating a maven plug in that prior to the compile phase reads the classpath and writes a file with the class names of the other modules in the classpath. Then ExternalProvides can read that file.

ok

@SentryMan SentryMan closed this Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants