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
|
I run:
and it runs java 21...is that expected? should it not tell me I don't have a java 21 with that vendor? |
|
sorry no, jdk-vendors is just of IFF the java version string isn't available. |
|
Jdk vendors is not used for running, it's only used for installing and listing jdks available for installation (yes, that flag should perhaps be moved to the list and install commands, but the config is global, and overriding the global config per command is somewhat complicated. I'll have to take a look how to improve that. For now it should at least not cause any issues) |
Ah no, we can't, because of course if you run Now, we'd probably prefer a theoretical new syntax where you could simply say |
|
trying to follow the logic here: 15 with zulu, 12 with termurin, 16 with temurin,zulu (or zulu,temurin) ..so this is showing all major java versions, if available, ordered by vendors? ...I see |
Yes, something like that. It's following the same we do right now. You have to imagine that by default our setting for vendors is "temurin,aoj" (preference given to temurin for being first in the list). And in that case doing When you ask for details no filtering is performed. |
|
@quintesse are the failures easily fixable or we wait? |
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?
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.
b37d7f4 to
7d835a7
Compare
7d835a7 to
48e9356
Compare
Fixes #2118