-
Notifications
You must be signed in to change notification settings - Fork 205
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
bugfix: Try and fix issue with Base64 reflection #1775
Conversation
Anyone willing to test it out on windows? Otherwise I can do that probably next week and run a new release. |
I tried releasing it locally on Windows, but it seems a bottomless rabbit hole :/ Try is a simple instruction: Spent an hour trying to google/fix that, but to no avail. I found Then it started failing with not finding cl.exe, which needed Visual Studio being installed (!!!) and it still didn't help. I think we will check this when the release runs :/ |
I honestly know nothing about what this is doing. Do you have any links or reference to what these are? |
There is a bit more here: https://www.graalvm.org/22.1/reference-manual/native-image/Reflection/ But I ma kind of guessing here and copying from dev-dirs, which seems to have that setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so this dirs-dev library tries to load java.util.Base64
by reflection here and to call getEncoder
which probably returns a java.util.Base64$Encoder
.
I wonder if we should also declare sun.misc.BASE64Encoder
here. Probably not if we compile with Java 11 or greater.
I cannot test this change now but it seems perfectly legitimate to me.
I will try and build it on CI, might be possible to download it and test on windows. |
That actually worked! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty, finally read through the link. Thanks! From what I can tell, yea this makes sense. LGTM
Based on https://github.com/dirs-dev/directories-jvm/blob/aa87fc1705fec69bc7f486dae6910306a50c221a/src/main/resources/META-INF/native-image/dev.dirs/reflect-config.json
Fixes #1718