feat: install multiple JDKs with same major version#2271
feat: install multiple JDKs with same major version#2271quintesse wants to merge 6 commits intojbangdev:mainfrom
Conversation
14d6edb to
9f510d6
Compare
|
Interesting. How's it to be used/managed? Couldn't spot clearly the user impact if any? |
|
The idea is that user impact at first is as close to zero as possible. Mainly to detect if the new system causes any backward compatibility issues. For the user that is accustomed to installing jdks with What does change is how jdks are stored on disk. Before you'd just have a bunch of numbered directories in But now with the new devkitman version you could see this: As you can see I just installed a new version 26. And it gets stored in a directory with a full name (its id) and additionally a symlink was created with the name "26" that points to the actual directory. This symlink serves two purposes: a) is serves as the "default" for that version, so if you have multiple version 26 jdks installed this symlink will point to your "preferred 26 version" and b) it maintains backward compatibility with older JBang versions that will only see the symlinks (older JBang versions ignore directories whose names are not integers). I still have to make a couple of changes before explaining what else has changed. I will keep you posted in follow-up messages here. |
|
gotcha. and i assume its fine but just asking to know if some limits - on windows the symbolic linking also works ? no extra rights needed etc? in past we "survived" these even without extra rights as jsut one folder but i know some stuff improved here i just forget which :) And wouldn't it be nicer if stored per vendor?, i.e. / ? or even to simplify have /installs//? just suggesting this to not mix-n-match too much in one folder directly. whats the signficance of "-jbang" in the name? |
JBang already needs those to work. The default jdk that gets added to your path is a link (a Junction to be precise).
I'm just somewhat following what I see others do. I also think it makes file/folder management slightly more complex to have more levels of directories. Given the fact that nobody should be looking directly at that folder and definitely not depend on its names and structure I feel it's okay to do it this way.
It's because the jdks are managed by the "jbang" provider. There's other providers that can create folders there, for example links to external jdks are handled by the "linked" provider and marked with "-linked". |
6decb1a to
db613f3
Compare
db613f3 to
3e306cf
Compare
3a2a8c8 to
8aeb565
Compare
|
@maxandersen this is now ready for review and devkitman 0.4.0 has been released so you don't need to build that to be able to test this PR. Given the fact that this is a pretty major change it must be tested quite well (I already did of course, but I might be "blind" to certain issues). It must also be tested that older JBang versions keep working even when newer JDKs get installed by this new version. Also, the workings and output of some of the jdk commands (eg |
|
NB: currently releasing 0.4.1 because of a test failure that only appeared when running JBang's GH CI. (Seems GH uses legacy DOS paths in their environment variables, eg. |
0ac3adf to
056cbeb
Compare
maxandersen
left a comment
There was a problem hiding this comment.
Should I ignore or honor the "do not merge" message ? :)
|
Sorry, this PR is ready for review but not ready to merge, I put it back to draft to make that clear. |
25edf97 to
f5e2801
Compare
|
Ok, I marked this PR ready for review again because everything works now. The integration tests are failing because of the Alpine test not being able to download the Jdk 17 it needs, but that's unrelated to this PR. |
f5e2801 to
fc85039
Compare
fc85039 to
2ad3d27
Compare
660d050 to
b13b12c
Compare
|
@quintesse any idea of why the new devkitman seem to fail handling idk 11 and other installs? |
|
seems to be wiremock related maybe? |
| import picocli.CommandLine; | ||
|
|
||
| public class JdkProvidersMixin { | ||
| protected static List<String> jdkProviders; |
There was a problem hiding this comment.
did you mean these to be static? they depend on arguments set on command line so sure can be static, but what if/when run during tests?
There was a problem hiding this comment.
But aren't the tests run sequentially?
Because if this is an issue we will have problems with all of these as well:
private static boolean verbose;
private static boolean quiet;
private static boolean offline;
private static boolean fresh;
private static boolean ignoreTransitiveRepositories;
private static boolean preview;
There was a problem hiding this comment.
Ah yes, I remember now why they are static. It's because the options can be set on different "levels", if you type jbang jdk --jdk-providers list or jbang jdk list --jdk-providers the option gets handled by a different instance of the JdkProvidersMixin (and you could even mix! Imagine jbang jdk --jdk-distros xxx list --jdk-providers yyy, one option would be set in one instance and the other in a second instance). This makes figuring out what options to actually use much harder than it needs to be. So switching to static "fixes" that.
It's definitely not thread-safe, but the cli commands are not designed to be.
Our project compiler/builder API is much more thread-safe in this respect, but it still uses the static fields I mentioned above. That might be something we'd want to fix at some point.
There was a problem hiding this comment.
but shouldn't we then use picocli @parentcommand or scopetype = inherited?
There was a problem hiding this comment.
opened quintesse#152 with pr that i think shows and fix the issue
Added a flag `--for-version` (or `-v`) to the `jdk default` command to allow setting the default Jdk to use for a specific major version. This is necessary now that we allow multiple Jdks of the same major version to be installed simultaneously.
Added some more parameter checks to `jdk install`. Also refactored `Jdk` class.
7d835a7 to
48e9356
Compare
48e9356 to
7692bb0
Compare
|
Ok, all green, So please go ahead, play around with it and break things :-) |
|
i can't get any of the commands to work... doesn't recognize the jdk-providers/jdk-distros ...can't see tests use them either..what am I doing wrong? |
what do you mean "doesn't recognize"? I can do the following without issue: just jbang jdk l --jdk-providers=javahome
just jbang jdk --jdk-providers=javahome l
just jbang run --jdk-providers=javahome echo@jbangdevAnd there are tests for |
|
ok, it does work now. i got caught up in jdk-vendors vs jdk-distros. good stuff! |
|
wait - didnt actually test the multi isntall... |
|
I ran bug? |
|
Ok, will look into that. But what would be the expected result here? Because I could think of two different interpretations:
There could even be a 3rd interpretation:
And depending on the other arguments you pass you might even expect a different result:
Option (1) might be difficult to explain, someone does Looking at this option (2) is perhaps the one easiest to explain and giving the least headaches? |
|
I assume if i say use Zulu 24 that 24 will give me Zulu. I think that's what you call option 2. But this also mean we need someway to say "don't put this as default" Maybe Jbang jdk install --extra 24 ? And somehow print what id to use if want to be explicit. |
|
Not sure, that It seems like a fix/workaround that's only useful in very specific situations that will be somewhat hard to explain when to use. Perhaps combining option (1) with a (THe problem might be: the default of what? There is a main/global default and a per-version default. DOes this option mean you want it to become the default for 24 or the main/global default?) |
|
How is the current PR intended to be used? it says install multiple jdks with same major version ...whats the command to do that if not |
|
Well the current PR doesn't yet give you all the options that I have in mind, because the changes were already big enough and I first wanted to get basic functionality working. Anyway, if you do And then you can run Right now we don't really show any other options, but you could easily imagine a Now that we have the basics working we just have to come up with good ideas and what options/features to add :-) |
|
For example, one of the things I want to allow is Imagine:
PD: I say |
Fixes #2118