-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Stable default location #986
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: adipiciu <[email protected]>
Signed-off-by: Nikolas Grottendieck <[email protected]>
…er>-<jre|jdk>` See adoptium#789 (comment) for details. Fixes adoptium#422 Signed-off-by: Nikolas Grottendieck <[email protected]>
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.
I'm good with this change as long as we send it out with a release note as it will deviate from the previous installs.
Is there some way/doc how to gather such requirements? (future release notes) |
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.
Just my 2 cents :
As already said, I'm think this is a bad idea to have only a fixed path with only major version as it can break some running process.
If done we must ensure :
- no running jvm are already running from the same folder. This PR does not include any change to secure that.
Or - warn the user that running jvm can be unstable, and ask to restart it asap.
Did the installer override/upgrade the previous version without asking/warning ?
It is OK to always use only major version in path if no previous install exists.
As suggested, two installer with different minor number and fixed path must be created to be able to test in place upgrade while running jvm.
I'm probably not able to make a reproducible test case where running jvm is affected by background change of files, but I'm certain I have already been impacted by that.
@douph1 this is a fair criticism and I am willing to help with getting it addressed, however, I am unable to building such logic into the installer by myself. If any of you can help with providing pointers etc., that'd be immensely helpful and I would try to incorporate this. |
I'm not sure we need to cover the case for the running JVM? IIRC - Typically with Windows installers the OS expects you to shut down the process of the application that you're upgrading. Perhaps a warning in the installer would be sufficient. |
As discussed in #789 I created a separate PR with effectively the same changes taking into account the proposal from #789 (comment).
The default location should now be
temurin-<ver>-<jdk|jre>
in the installer with these changes.I did not find any variable / localization reference in the installer code base that contains the required
temurin
string. I can change that from the current hardcoded string, of course, but would need a little input/guidance on what to do.