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

Use ELK's classloader, not root classloader to discover services #1109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahus1
Copy link

@ahus1 ahus1 commented Dec 23, 2024

This will use the LayoutMetaDataService's classloader to allow for cases when this JAR and other ELK JARs are loaded in dynamic classloader.

The previous logic introduced in #789 would only work if the classes were part of the root classloader.

Closes #1108

@soerendomroes
Copy link
Contributor

We will take a look at this issue next year such that this can probably be included in the upcoming 0.10.0 release.

@ahus1
Copy link
Author

ahus1 commented Dec 23, 2024

@soerendomroes - thank you for the first feedback and running the CI. I'll see if I can make the CI green for this PR in the meantime.

To add some more context: This would allow ELK to be running in with PlantUML when packaged for example in the IntellIJ AsciiDoc plugin that I maintain. I also assume it would enable the PlantUML Server.

@ahus1
Copy link
Author

ahus1 commented Dec 23, 2024

Update on my previous comment: It seems that this fails for the elkjs build. Not sure what is the best way to tackle this.

I assume the serviceloader is not used at all in JS, so a similar way to override the class for JS is the way similar to SVGImage? I didn't try it out yet, some advice would be great on how to proceed here. Or that someone with more experience with elkjs takes over on that part.

@soerendomroes
Copy link
Contributor

@ahus1 Probably one can just exclude the assignment similar to this.

@ahus1 ahus1 marked this pull request as draft December 23, 2024 15:18
@ahus1 ahus1 force-pushed the is-1108-use-elk-classloader branch from 904bff4 to 7b03d3c Compare December 23, 2024 15:47
@ahus1
Copy link
Author

ahus1 commented Dec 23, 2024

Thank you @soerendomroes - I now see the pattern :-D

I pushed a change that now made the CI pass green in my fork (https://github.com/ahus1/elk/actions/runs/12469241806 + https://github.com/ahus1/elk/actions/runs/12469241788)

@ahus1 ahus1 marked this pull request as ready for review December 23, 2024 15:49
@ahus1 ahus1 requested a review from soerendomroes December 23, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants