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

Write to different services path #212

Merged
merged 3 commits into from
Jun 23, 2024
Merged

Write to different services path #212

merged 3 commits into from
Jun 23, 2024

Conversation

SentryMan
Copy link
Collaborator

relies on avaje/avaje-spi-service#25 to register META-INF services

@SentryMan SentryMan self-assigned this Jun 21, 2024
@SentryMan SentryMan added this to the 2.0 milestone Jun 21, 2024
@rbygrave
Copy link
Contributor

Hmmm, it seems to me with this PR the validation-generator AP using generated-services will then clash with the avaje-spi-service AP? I'm missing something ...

@SentryMan
Copy link
Collaborator Author

it's illegal to write to the same file twice, but reading has no such problem

@SentryMan
Copy link
Collaborator Author

so what's happening here is that validator is writing to META-INF/generated-services/ValidatorExtension. Avaje spi does only read operations on the META-INF/generated-services folder, so spi is free to write to META-INF/services/ValidatorExtension as no other processor contests that file

@rbygrave
Copy link
Contributor

So the avaje-validation-generator has a hard dependency on avaje-spi-core (annotation processor).

So it has gone from letting avaje-spi-core validate the module-info to being a hard dependency required by the avaje-validation-generator.

@rbygrave
Copy link
Contributor

What we get avaje-spi-core to do is merge multiple implementations of a SPI interface in the appropriate META-INF/service file.

It does look like the implication is that IF an application desires explicit control over the META-INF/service file then they need to disable the avaje-spi-core AP. The reason this has got a lot more "interesting" is because in terms of META-INF/service files we now need a lot more "merging" of contents in those files due to adopting the pattern of common spi interfaces when previously we were unlikely to have a clash. That is, previously people could define spi service implementations manually/traditionally if they desired.

That is, as a consequence of adopting the pattern of common spi interfaces is that we now are MUCH more likely to need content merging those META-INF/services files (in fact maybe this is now really common). IF people are willing to use avaje-spi-service then it can do that merging for us - effectively merging in the contents from META-INF/generated-services files to output into META-INF/services the merged list of services.

IF the use of avaje-spi-service @Service @ServiceProvider doesn't work for people and they need to control this explicitly themselves, then they need to disable the avaje-spi-core AP.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Jun 23, 2024

META-INF/service file then they need to disable the avaje-spi-core AP.

You misunderstand, since user defined services are in a different directory (src/main/resources instead of target/classes) it is also exempt, and not affected. You can observe this behavior in the black box tests of avaje spi. (We register one of the services via src/main/resources in addition to the regular test cases)

EDIT: wait a second...

@SentryMan
Copy link
Collaborator Author

So the avaje-validation-generator has a hard dependency

You got me there though, I can change this pr to only take effect when avaje spi is detected

@SentryMan SentryMan requested a review from rbygrave June 23, 2024 03:18
@SentryMan
Copy link
Collaborator Author

SentryMan commented Jun 23, 2024

EDIT: wait a second...

Ok, so I'm still correct, there is still no contention because resources are copied from src/main/resources to target before compilation. (the copy doesn't count as a write) The only thing now is that spi seems to have lost the ability to detect and validate manually registered spi interfaces, so gotta fix that

@rbygrave rbygrave merged commit 339bf46 into avaje:main Jun 23, 2024
6 checks passed
@SentryMan SentryMan deleted the spi-dir branch June 23, 2024 19:22
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