-
Notifications
You must be signed in to change notification settings - Fork 159
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
feat: add jbang jdk install <version> <path> #1252
feat: add jbang jdk install <version> <path> #1252
Conversation
nandorholozsnyak
commented
Feb 23, 2022
- Ability to link pre-installed JDK directory to JBang
- Documentation update
- Ability to link pre-installed JDK directory to JBang - Documentation update
Codecov Report
@@ Coverage Diff @@
## main #1252 +/- ##
============================================
+ Coverage 58.28% 59.01% +0.73%
- Complexity 1077 1139 +62
============================================
Files 86 96 +10
Lines 5818 5995 +177
Branches 996 996
============================================
+ Hits 3391 3538 +147
- Misses 1925 1950 +25
- Partials 502 507 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Looks good in general! Good job!
@@ -56,6 +56,9 @@ Installs a JDK. | |||
_version_:: | |||
The version to install | |||
|
|||
_[existingJdkPath]_:: | |||
Already existing JDK's path on the filesystem. If path is given, its version will be checked against the incoming one, if not the same, the process will not finish successfully. |
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.
A minor thing but I think it's best to rewrite this using a positive view. Eg. ".. and if the same, the given JDK will be added successfully". (The use of "added" is somewhat vague and ambiguous, perhaps there is a better word?)
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.
added could be linked maybe?
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.
"linked" is good 👍
* @param path path to the pre-installed JDK. | ||
* @param version requested version to link. | ||
*/ | ||
public static void linkToExistingDirectory(String path, int version) { |
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.
Minor: I'd prefer "linkToExistingJdk"
if (Files.exists(jdkPath)) { | ||
Util.verboseMsg("JBang managed JDK already exists, must be deleted to make sure linking works"); | ||
try { | ||
FileUtils.deleteDirectory(jdkPath.toFile()); |
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.
Don't use this, use our Util.deletePath()
, that already handles any errors (or you can tell it to be quiet).
@@ -203,6 +207,40 @@ public static void uninstallJdk(int version) { | |||
} | |||
} | |||
|
|||
/** | |||
* Links JBang JDK folder to an already existing JDK path with a soft link. It |
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.
Don't specify "soft", because the linking code will actually create a hard link if it can't create a soft link for some reason.
Util.infoMsg("JDK " + version + " has been linked to: " + linkedJdkPath); | ||
} else { | ||
throw new ExitException(EXIT_INVALID_INPUT, "Java version in given path: " + path | ||
+ " is not having the requested version " + version + ", please provide a proper path"); |
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.
say " is X, which does not match the requested version Y". Remove the "please provide..." part.
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.
It may not be able to infer the X value, cause this path could point to anywhere, it can have no Java inside it, I can do an evaluation and if it does not work then fallback to the original message if it is okay.
@@ -81,6 +81,7 @@ private static int determineJavaVersion(Path javaCmd) { | |||
String output = Util.runCommand(javaCmd.toString(), "-version"); | |||
int version = parseJavaOutput(output); | |||
if (version == 0) { | |||
Util.verboseMsg("Version is 0, reading it from java.version property"); |
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.
say something like "Version could not be determined from: '$javaCmd -version', trying 'java.version' property"
@@ -104,7 +105,7 @@ public static Path getJdkHome() { | |||
public static int parseJavaOutput(String output) { | |||
if (output != null) { | |||
Matcher m = javaVersionPattern.matcher(output); | |||
if (m.find() && m.groupCount() == 2) { | |||
if (m.find() && m.groupCount() == 1) { |
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.
That does indeed seem to have been wrong all this time, good catch!
Oh and add "Fixes #1248" to the end of the commit message, then GitHub will automatically close the issue when your PR gets merged (you probably know that and just forgot 😄) |
- Changes for PR - Fixes jbangdev#1248
Mentioned things are fixed, I'm coming soon with the tests. |
- Tests for new feature
Coverage should be a bit better now. |
- Change in test
Any cool idea how to fix this error happening with the Windows build?
|
- Extra use cases
Is that error for real? Because that path looks very wrong! There are URLs and what not inside of it. |
Hm, on a Windows based laptop the build is fine. Any guesses to the TempDir? |
This one :-)
|
Sorry I still had no time to finalize the wrong tests, I ran the tests on Linux and on Windows too. I think there were some reasons why this part was not having tests :D @quintesse do you have any tips to resolve the problem? What could be causing the problem on the CI? |
Well actually I think the weird path must have been a copy&paste issue on your part, because if I look at the logs it looks normal:
Edit: actually it's an issue with copy&pasting from the logs. Somewhere GitHub decided to insert links for things it thinks are commit ids. ANyway, I cleaned it up and the above logs are displayed correctly now. Edit2: Haven't had time to look at why it's failing though. |
- Disabling tests on Windows that are using symlinks
On windows the symlinks are a bit clunky but I think you are aware of that, so I used |
I thought we do symbolic linking for jdk install testing elsewhere so what is special about this usecase? |
I do not really see any tests that are installing the JDKs (of course there are but not in the TestJdk class). What I could basically debug is that the creation of hard links on Windows works only if you link one file, not a whole directory to another (like in the TestEdit test cases). As I read, hard linking folder on Windows required admin privileges or different policies, so this current folder/directory linking might not be the best solution for this particular feature, but on Linux it works fine. Should I come up with some other alternative ? |
- Changes in test and reason for disabling test on Windows
i'll need to test on my windows setup but I thought we used linking for jdks - but if we dont we might need a "jdklocations.json" instead of 100% file path based solution. |
I was also thinking on a config file to locate JDKs, it should work everywhere. Let me know if I should rework it |
I think that could be useful. |
- CI config change to check what is the output of the tests, will get reverted
BTW, so if you are using a privileged powershell or command line then the whole feature works on Windows too. The symbolic linking on Windows has been already announced as it could only work with admin privileges only. Knowing this as an already announced limitation on Windows, should it be still reworked to the config file based JDK location? I just changed the CI build config to see the more information in the logs, I will revert it as soon as I just understand the situation within the CI build. |
- Retesting old version with custom JBANG_CACHE_IDR
- Retesting old version with custom JBANG_CACHE_IDR
- Retesting old version with custom JBANG_CACHE_IDR
symbolic link was supposed to only affect jbang edit; for jdk location that is used for running i think we need to find a way to support it across platforms. |
Allright, I was not able to reproduce the error that was happening in multiple builds... I'm going to rework it with a config file. |
a reason we didnt do config file before was that the jdk mechanism needs to work in the various bash/powershell/cmd script too - so we need to have that in mind how that will work. |
@maxandersen This config based change could be very "big", I would be totally fine to use admin privileges to set and install JDKs because of that limition, so what is going to be the final decision? Keep it as is, cause it has been stated earlier or should I do the refact on it? I have no Mac to test, but I can imagine it does not have this limitation only Windows. |
hmm okey - if windows already has this issue then maybe better we add this now (with its known limitations) and look at config file based approach separately. @quintesse wdyt ? you know the nastiness around these |
- Solution using BaseTest prepared JBANG_CACHE_DIR - Removing --info from gradle build
I'll take a look tomorrow. But a quick remark would be that we do indeed use symlinks on all platforms and that has always worked. Using any kind of "index" will make the whole "default" system not work. (You could never use a path like that for PATH or JAVA_HOME, you'd have to change them each time you switch the default JDK). |
Yes, but there are two separate but related features here. Jbang jdk install to point to a specific alternate java vm to use when resolving java version. Jbang jdk default install location. We can do the first via config file and second can keep using symlink and be limited |
Sure, we could, but we're already using the linking on Windows and we already accept that in certain cases the linking on Windows isn't ideal. So why introduce a new system just to work around one of those issues and not both? It seems to me that a better investment of time for the future would be to figure out how others are able to reliably make linking work on Windows (eg. Scoop). |
Btw, for me this PR works perfectly on Windows 👍 🙂 |
src/main/java/dev/jbang/cli/Jdk.java
Outdated
throws IOException { | ||
if (force || !JdkManager.isInstalledJdk(version)) { | ||
JdkManager.downloadAndInstallJdk(version); | ||
if (StringUtils.isNotBlank(path)) { |
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.
Prefer the usage of https://github.com/jbangdev/jbang/blob/238ef90b0f65aba896b241b06bc8cde796364382/src/main/java/dev/jbang/util/Util.java#L1339_L1349 over the use of external libraries. (This might change in the future but at least that way it would be easy to find all occurrences)
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.
LGTM. The comments I left are mostly clean-up.
@@ -31,10 +31,11 @@ | |||
@BeforeEach | |||
void initEnv(@TempDir Path tempPath) throws IOException { | |||
jbangTempDir = Files.createDirectory(tempPath.resolve("jbang")); | |||
jbangTempCacheDir = jbangTempDir.resolve("cache"); |
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.
The changes in this file are not necessary (and unwanted really). See my comments on TestJdk.
|
||
final File testCache = initJBangCacheDir(); | ||
final File jdkPath = new File(testCache, "jdks"); | ||
final File jdkPath = new File(jbangTempCacheDir.toFile(), "jdks"); |
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.
Use JdkManager.getJdksPath()
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.
Hehe, that is a private method. I can make it public to make it work.
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.
Yeah, probably okay to do that 👍
void testJdkInstallWithLinkingToExistingJdkPathWhenJBangManagedVersionDoesNotExist(@TempDir File javaDir) | ||
throws IOException { | ||
initMockJdkDir(javaDir); | ||
final File jdkPath = new File(jbangTempCacheDir.toFile(), "jdks"); |
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.
Same here, use JdkManager.getJdksPath()
Personally I'd merge this PR as-is because it works great, even on Windows as long as the user has made the appropriate policy changes (which were already necessary on Windows). We can then think about how to improve the linking issue later. |
I will fix the findings after work. Thank you. |
- Removing StringUtils usage in Jdk - Rework test to use JdkManager's method
- Fixing failing test case
Everything should be fixed an the builds should be working. |
Good job, thanks @nandorholozsnyak ! |
@all-contributors please add @nandorholozsnyak for code |
@nandorholozsnyak already contributed before to code |