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

add BuildServicesResolver.setBuildServices() #617

Merged
merged 1 commit into from
May 25, 2022

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented May 24, 2022

No description provided.

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ladicek should we also include the policy to only be able to set the impl only once?
We do the same thing for CDI providers.

@Ladicek
Copy link
Contributor Author

Ladicek commented May 24, 2022

@Ladicek should we also include the policy to only be able to set the impl only once? We do the same thing for CDI providers.

I considered that, but contrary to your statement (and contrary to the javadoc you link to), I didn't find CDI.setCDIProvider to check this.

@manovotn
Copy link
Contributor

@Ladicek should we also include the policy to only be able to set the impl only once? We do the same thing for CDI providers.

I considered that, but contrary to your statement (and contrary to the javadoc you link to), I didn't find CDI.setCDIProvider to check this.

Indeed, you're correct. I only read the javadoc and assumed that.
It seems like a bug TBF

@Ladicek Ladicek force-pushed the set-build-services branch from 92aa067 to 3b56b49 Compare May 24, 2022 09:01
@@ -48,7 +55,7 @@ private static void discoverFactories() {
BuildServices.class, BuildServicesResolver.class.getClassLoader());

if (!loader.iterator().hasNext()) {
throw new IllegalStateException("Unable to locate AnnotationBuilderFactory implementation");
throw new IllegalStateException("Unable to locate BuildServices implementation");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new IllegalStateException("Unable to locate BuildServices implementation");
throw new IllegalStateException("Unable to locate a BuildServices implementation");

@manovotn
Copy link
Contributor

manovotn commented May 24, 2022

This change was discussed during the meeting and agreed on it as a way forward.
We want to keep it as similar to how setting CDI provider works as possible (including the current CL used for loading the services).

@arjantijms do you need a service release with this fix or are you going to use the workaround mentioned in the issue?

@arjantijms
Copy link

do you need a service release with this fix or are you going to use the workaround mentioned in the issue?

I will use the workaround mentioned in the issue for now, so a service release is not strictly needed from my part.

@manovotn manovotn merged commit 0b7c10a into jakartaee:master May 25, 2022
@Ladicek Ladicek deleted the set-build-services branch May 25, 2022 16:33
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.

Osgi environments (like GlassFish) have trouble with BuildServicesResolver
5 participants