Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Okeanos
Copy link

@Okeanos Okeanos commented Aug 23, 2024

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.

@karianna karianna requested review from sxa, douph1 and gdams August 25, 2024 05:10
Copy link
Contributor

@karianna karianna left a 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.

@judovana
Copy link

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)

Copy link
Contributor

@douph1 douph1 left a 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.

@Okeanos
Copy link
Author

Okeanos commented Aug 29, 2024

@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.

@karianna
Copy link
Contributor

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants