feat: linuxdistro jdk provider #1826
Conversation
adds `linuxdistro` provider which for now searches `/usr/lib/jvm` for jdks. It is not enabled by default as it is not possible to safely determine the version and build of the jdk present or needed to be installed if mising. Meaning to get it user need to select it `--jdk-provider=linuxdistro` or just enable all providers `--jdk-provider=all` on command line or run `jbang config set jdk-provider linuxdistro` to make it default for all jbang invocations.
Why not? Is there no RELEASE file in those JDKs?
Not sure what you mean by this?
It shows what the providers tell it to show. Providers shouldn't return anything without doing sanity checks. |
I mean, once these are enabled we no longer have a consistent java version when using jbang by default. Today its always, honor your configured path, otherwise fallback to .jbang managed or foojay fetched jdks. if we enable the others like sdkman/linux alternatives you will get whatever order of providers are enabled and used on your system and when jbang perform installs it might not be the one selected next time you run - even in the same shell. unless jbang managed ones trumph everything else I reckon. |
Ah yes, indeed. Although I'd say that given what you mention next:
the fact that we first use what's on the path already means that we can never be sure what JDK we're running.
jbang managed ones do indeed have priority (after what's on your PATH that is, like mentioned above) |
|
Btw, talking about native providers... you run MacOS, can't you make a provider for JDKs installed on Macs? :-) |
| @@ -121,6 +121,8 @@ default Jdk createJdk(@Nonnull String id, @Nullable Path home, @Nonnull String v | |||
|
|
|||
| default String name() { | |||
| String nm = getClass().getSimpleName(); | |||
| // TODO: this 11 is here assuming it ends in "JdkProvider" - dont make that | |||
There was a problem hiding this comment.
Why not? It's a reasonable default implementation, isn't it? If your implementation doesn't follow that scheme you can simply override this method and return the value that you desire.
There was a problem hiding this comment.
I had a name called LinxDistroProvider and it showed up as 'linuxdist' which confused me - will update one that is a bit more explicit.
I see now what you were trying to do. You're using the |
macs doesn't install native jdks anymore do they ? |
we can be sure the user gets the JDK that is defined in his terminal/session like any other common cli command. |
not sure where to best add it as some of the code already does use it for naming ...but other seems to use filenames. lets do that in another pr. |
What I mean is that we cannot make assumptions about the JDK we're running, either we're running a JBang-managed JDK or ... something else that we know very little about. So what's so different about running another JDK that's available on the system (as long as we can verify it's the correct versions and it's a JDK)? In what way are managed JBang JDKs "better" than what the user has on their system? (I'm of course assuming modern JDKs from any vendor are fully functional) |
Dunno, I don't use Macs so I have no idea how people install JDKs there :-) |
| if(nm.endsWith("JdkProvider")) { | ||
| return nm.substring(0, nm.length() - 11).toLowerCase(); | ||
| } else { | ||
| return nm; |
There was a problem hiding this comment.
Should probably do .toLowerCase() here
quintesse
changed the title
maxandersen
force-pushed
the
linuxdistroprovider
branch
2 times, most recently
from
901c620
to
3761694
Compare
maxandersen
force-pushed
the
linuxdistroprovider
branch
from
3761694
to
a269735
Compare
There was a problem hiding this comment.
Actually, I've just tested this myself and I do see the problem now. There's a whole bunch of duplication going on. I think what we should do is add a protected boolean acceptFolder(Path jdkFolder) { return true; } to the BaseFoldersJdkProvider which subclasses can override. In the case of the LinuxDistro provider I'd then check that the folders are not symlinks, and a bin/javac exists (in my case all folders are JREs)
NB: we could make this test the default implementation as well, because realistically all JDKs should be able to pass these tests.
|
@quintesse is that not done better in separate PR ? |
Well , right now this PR would add a dozen unusable entries on my system. That's not really something we want, right? |
|
@quintesse i don't see them so if you can push update to this pr with what you wanna see fixed lets do that. |
|
Will do, but on PTO now, Have to see when I have a bit of time :-) |
| @Nullable | ||
| @Override | ||
| protected String jdkId(String name) { | ||
| return name + "-nixdistro"; |
There was a problem hiding this comment.
Is there a way you chose a different name here than the name of the provider itself? ("linuxdistro" vs "nixdistro")
quintesse
force-pushed
the
linuxdistroprovider
branch
2 times, most recently
from
dab4045
to
ba3c08e
Compare
quintesse
force-pushed
the
linuxdistroprovider
branch
from
ba3c08e
to
f2f82b3
Compare
adds
linuxdistroprovider which for now searches/usr/lib/jvmfor jdks.It is not enabled by default as it is not possible to safely determine the version and build of the jdk
present or needed to be installed if mising.
Meaning to get it user need to run
jbang config set jdkproviders allorjbang config set jdkproviders linuxdistroif only want this.You can see which providers are finding jdks by running
jbang jdk list --show-detailsFixes #1825
@quintesse I see that
jbang jdk list --show-detailslist what looks like all dirs even though javac is not available. is that expected?