refactor: Separated JDK management code into its own module #1874
Conversation
quintesse
force-pushed
the
jdkman_lib
branch
from
afc114d to
f57b1c6
Compare
quintesse
changed the title
| private static JdkManager jdkManager = null; | ||
|
|
||
| @Nonnull | ||
| public static JdkManager jdkManager() { |
There was a problem hiding this comment.
we seem to call this in many places - but how is going to take into affect jbang provided config of i.e. providers?
There was a problem hiding this comment.
Right now the idea is that the config gets used exactly in that file you're referencing. So the builder will use the config to initialize the jdk manager, including the providers. The jdk manager itself is then completely agnostic where its configuration comes from.
There was a problem hiding this comment.
ok so in jbang cli we would not call jdkmanger() but have our own instance configured from jbang config?
There was a problem hiding this comment.
Well, this JavaUtil class is in the jbang CLI, so this code here is exactly where that instance using jbang config is created.
There was a problem hiding this comment.
NB: If you look a bit lower in the createProvider() method you see it references config, which is the jbang config. The missing part I mentioned in our chat is exactly that: tying together the config with the instance creation :-)
quintesse
force-pushed
the
jdkman_lib
branch
2 times, most recently
from
8c7ba7e to
4489268
Compare
quintesse
force-pushed
the
jdkman_lib
branch
2 times, most recently
from
2d66e04 to
21e1e5a
Compare
quintesse
force-pushed
the
jdkman_lib
branch
5 times, most recently
from
260870f to
0f35aa5
Compare
quintesse
force-pushed
the
jdkman_lib
branch
7 times, most recently
from
e0639c0 to
f5e2bb3
Compare
quintesse
force-pushed
the
jdkman_lib
branch
5 times, most recently
from
4083f67 to
1d133d2
Compare
|
small thing: how come the package is called |
| //remove this to see all the missing tags/parameters. | ||
| options.addStringOption('Xdoclint:none', '-quiet') | ||
| } | ||
| allprojects { |
There was a problem hiding this comment.
no matter what I think the changes to enable additional modules in this repo should stay. enable us to split up or add things where it makes the most sense.
There was a problem hiding this comment.
Any changes I made to the gradle build file are there only to get things to work, which was already difficult enough. Very likely I made mistakes or did it in an improper way.
| * This JUL Formatter is used to format JUL log messages in such a way that they | ||
| * look similar to JBang log messages. | ||
| */ | ||
| public class JBangFormatter extends Formatter { |
There was a problem hiding this comment.
hmm - originally I explicitly kept "ux output" separate from "logging output" ...is there a reason why we want to redirect all output to JUL ?
There was a problem hiding this comment.
Well, you might not want all of the logging output to be confused with UX output, I can understand that.
But there are two things that come to mind:
- our output mechanism sure does look like logging (having levels like warn, inifo, error, debug, etc)
- to make the new module really independent from JBang I needed a way to generate output that didn't use JBang's system and I didn't want to introduce some new interface
So that's why I decided to switch the module's output to use plain logging but then the output would look different from before, so that's why I decided to capture the logging output and make it look like JBang output.
Now, I thought it would be nice to do the same for anything that could be outputting logging info, but that's just something I decided in the moment and we could easily change that to only work for the module and nothing else.
| @@ -0,0 +1,4 @@ | |||
| handlers=java.util.logging.ConsoleHandler | |||
There was a problem hiding this comment.
why do we need this now? (not against it - just want to understand the rationale)
There was a problem hiding this comment.
It's just to make any logging appear as if it comes directly from JBang (with our normal "[jbang]" prefix for every line), which was needed to make the logging output from the new module appear exactly the same as it did before.
Historical reasons, because that was the tool is called that I want to use this module for. But it can of course be anything we want. |
Fixes #1857