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

Changed INSTALLDIR to fixed path to fix #422 and #600 #789

Closed
wants to merge 3 commits into from

Conversation

adipiciu
Copy link
Contributor

@adipiciu adipiciu commented Jan 7, 2024

By using only the Major version in the installdir path, the installation directory will not change during upgrades.

C:\Program Files\Eclipse Adoptium\jre-8-hotspot
C:\Program Files\Eclipse Adoptium\jdk-8-hotspot
C:\Program Files\Eclipse Adoptium\jre-11-hotspot
etc..

@adipiciu
Copy link
Contributor Author

adipiciu commented Jan 7, 2024

Zulu, Liberica and Corretto are also using only the major version in the installdir path.

@karianna
Copy link
Contributor

karianna commented Jan 7, 2024

what happens in the upgrade use case for existing installs?

@adipiciu
Copy link
Contributor Author

adipiciu commented Jan 8, 2024

what happens in the upgrade use case for existing installs?

The same thing as before, the installdir changes to the new one with a different version. But with this change, the installdir will remain fixed in the future.

@karianna karianna requested review from gdams and douph1 January 8, 2024 21:05
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this at the PMC today and generally agreed with this approach, however we should consider whether jdk-xx-hotspot is the right directory name for this or if we can select something better, so I'm opening this question to interested parties as to what the correct top level directory name should be.

@sxa
Copy link
Member

sxa commented Jan 31, 2024

Comparing with Linux, which installs under /usr/lib/jvm and has directories named:

temurin-17-jdk-amd64 (.deb)
temurin-17-jdk (.rpm)

On that basis it feels like a format change to temurin-xx-jdk etc. for windows would be appropriate - what do others think? Noting that while the Windows builds are using hotspot our build system typically has temurin as a value for VARIANT which is explicitly distint from hotspot so this would potentially be preferable.

@sxa sxa removed the PMC-agenda label Jan 31, 2024
@adipiciu
Copy link
Contributor Author

@sxa 'temurin-17-jdk' seems fine for me. But there is no temurin JVM variant. See #438

@adipiciu
Copy link
Contributor Author

adipiciu commented Jan 31, 2024

Actually, Eclipse Adoptium is already in the path, so temurin is redundant. Right now, as mentioned here #2671, temurin = hotspot, but this was just a workaround. You should rely on the VENDOR variable to build a vanilla JDK, not on the JVM variant...
So the 'temurin-17-jdk' option is wrong.

@sxa
Copy link
Member

sxa commented Jan 31, 2024

as mentioned here adoptium/temurin-build#2671, temurin = hotspot, but this was just a workaround

Yep - it was me that implemented that change - VARIANT was just the easiest differentiator in the build process as it can also be set to things like dragonwell, corretto, bisheng which are all fundamentally hotspot based, as well as openj9 which isn't.

@sxa 'temurin-17-jdk' seems fine for me. But there is no temurin JVM variant. See #438

You're right that there is no difference in the JIT (other than anything arising from the compilers we use, and the compiler options) however Eclipse Adoptium is the vendor so it's right that the top level directory has that, but Temurin is our brand of openjdk so there's a good argument to have that in the directory path too IMHO to make the name visible, especially if it gives us consistency in the naming with the Linux installers.

(Note - I'm willing to be convinced either way, but I'm probably 80% towards going for temurin in the name and I'm interested to hear everyone's opinions on it :-) )

@douph1
Copy link
Contributor

douph1 commented Feb 12, 2024

What will happen to the running JVM when you delete/add/replace files/libs from same folder of existing process ?

@steelhead31
Copy link
Contributor

steelhead31 commented Feb 13, 2024

I'm assuming this is just a default path shown in the MSI installer, and as such is still available to be overridden at install time, as its definitely useful to be able to install multiple versions of the same major release for testing/evaluating upgrades.

I think I agree with @sxa , I'd like temurin in the name too, .. the default path in the most recent JDK21 installer is

c:\program files\Eclipse Adoptium\jdk21.0.2.13-hotspot

adjusting this to be c:\program files\Eclipse Adoptium\temurin-21-jdk would bring consistency with the other platforms. Its only a default/suggestion, so I guess its always

@douph1
Copy link
Contributor

douph1 commented Feb 16, 2024

I think I remember that some time ago Oracle installer ask user to close running jvm before upgrade in place.
We can't do that (because we don't know how).

Why are older Oracle installer was doing that before ? To not have a running jvm with some new files and some old files, or because some files where locked and cannot be updated in place.
I think MSI can handle later upgrade of locked file on restart.. but then the goal is not achieved until restart.

I think we must build some installer with major version (only) in path to allow to test upgrade while running jvm before merging into master.

@Okeanos
Copy link

Okeanos commented Aug 21, 2024

As the one who originally created #422 and looks very much forward to getting this solved, I am wondering what has to happen so this PR gets looked at & approved?

@karianna
Copy link
Contributor

Also see #983

@karianna
Copy link
Contributor

@adipiciu Would you be able to resolve the merge conflict / rebase?

@sxa
Copy link
Member

sxa commented Aug 22, 2024

Zulu, Liberica and Corretto are also using only the major version in the installdir path.

Noting that Red Hat's installer is the same as Temurin (It goes into C:\Program Files\RedHat\java-21-openjdk-21.0.4.0.7-1 for the latest release), so there is precedent both ways

@adipiciu
Copy link
Contributor Author

@karianna Sorry, you can close this PR. I can't wait 1 year for these guys to take a decision.
@Okeanos You can use msiexec /i "msifilename.msi" INSTALLDIR="C:\Program Files\Eclipse Adoptium\jre-8-hotspot" or whatever fixed name you want. Or even use Zulu, Corretto or Liberica builds which already have fixed paths.

@adipiciu adipiciu closed this Aug 22, 2024
@Okeanos
Copy link

Okeanos commented Aug 22, 2024

@adipiciu thanks for initially working on this first of all and thank you also for your suggestion. I know that I can override the default location when invoking the installer (and routinely do so). My goal was to make all of this smoother for automated processes, especially considering e.g. winget where it is not guaranteed that the Temurin manifest exposes this switch all the time.

Would it be okay for me to reuse the changes you proposed in your PR to create a new one? I would attribute these changes to you, of course.

@karianna, @douph1, @sxa could you give us a rough outline here what still has to be decided/what official outcomes are which need to be incorporated into a new PR?

@karianna
Copy link
Contributor

@adipiciu thanks for initially working on this first of all and thank you also for your suggestion. I know that I can override the default location when invoking the installer (and routinely do so). My goal was to make all of this smoother for automated processes, especially considering e.g. winget where it is not guaranteed that the Temurin manifest exposes this switch all the time.

Would it be okay for me to reuse the changes you proposed in your PR to create a new one? I would attribute these changes to you, of course.

@karianna, @douph1, @sxa could you give us a rough outline here what still has to be decided/what official outcomes are which need to be incorporated into a new PR?

That would be welcome - I think given #983 let's start with what was proposed here: #789 (comment)

Okeanos added a commit to Okeanos/adoptium-installer that referenced this pull request Aug 23, 2024
Okeanos added a commit to Okeanos/adoptium-installer that referenced this pull request Aug 23, 2024
…er>-<jre|jdk>`

See adoptium#789 (comment) for
details.

Fixes adoptium#422

Signed-off-by: Nikolas Grottendieck <[email protected]>
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.

7 participants