Handle missing default VMInstall when querying JVM system-packages #722
Conversation
build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java
Outdated
Show resolved
Hide resolved
Test Results 246 files + 6 246 suites +6 57m 4s ⏱️ + 8m 23s For more details on these errors, see this check. Results for commit 70cb73f. ± Comparison against base commit ac4df5b. This pull request removes 1 test. |
|
@akurtakov, @HannesWell : tests are green, I would propose to merge and re-trigger IBuild for RC1. |
Agree on merging. But isn't a I-Build triggered any way in about two hours? |
Looking at the calender there is indeed no more I-build planned for RC1. So I agree on both. :) |
|
Please merge and trigger I-build. |
Thanks. And thank you Andrey and Gireesh for your reviews! |
| if (targetVM == null) { | ||
| LOGGER.warn("No default JRE installation selected"); //$NON-NLS-1$ | ||
| return List.of(); | ||
| } | ||
| Collection<String> targetVMSystemPackages = querySystemPackages(targetVM, null); |
There was a problem hiding this comment.
I would rather let querySystemPackages return a stream instead instead it is even converted to a stream right after that... and having null as return value for collections is really ugly....
Fixes #721
This handles the case that
JavaRuntime.getDefaultVMInstall()returns null, which according to the docs happenswhen no defaultVM was set or when the default VM has been disposed.I also considered to just select any of the available VMs, maybe the 'latest' one, but since no default JRE should actually not happen (at least I get an error in the preferences if I don't select one), for me it seems to be better to warn the user about that instead of selecting just any VM.
@akurtakov should we merge this now to have this in RC1 or better wait for RC2?