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

Updated msi windows installers to use wix v5 #851

Merged
merged 72 commits into from
May 9, 2024

Conversation

jmjaffe37
Copy link
Contributor

@jmjaffe37 jmjaffe37 commented Apr 10, 2024

In moving to wix v5, some of the code could be simplified a bit (mainly: light.exe and candle.exe commands could be combined into one). For end users to use this wix installer creation tool, they will just need to run:
dotnet tool install --global wix --version 5.0.0

To install wix 5.0.0 (or they can install another version of wix v4 or wix v5 if they so choose).

@jmjaffe37
Copy link
Contributor Author

jmjaffe37 commented Apr 10, 2024

Notes on my testing:
I ensured that both OpenJDK and Microsoft MSI installers are able to be created for all supported versions. I downloaded the resulting MSIs, installed it, checked that the java --version string was correct, and then uninstalled it (with the installer) and made sure that it was gone.

Link to successful run on my fork (although I believe that this PR will also do the same checks): https://github.com/jmjaffe37/openjdk-installer/actions/runs/8624015173

@jmjaffe37 jmjaffe37 marked this pull request as draft April 10, 2024 17:43
@jmjaffe37
Copy link
Contributor Author

Converted to draft due to ongoing release. Note: currently passing all PR checks

@jmjaffe37
Copy link
Contributor Author

Note: I had to change the xml propery ID ALLUSERS in Main.wxs.template to ALLUSERSSETUP to avoid a duplicate symbol error that I was getting locally. Here is a link to a run I did on github actions to recreate this error that I previously had locally:
https://github.com/jmjaffe37/openjdk-installer/actions/runs/8635538967/job/23673480230#step:7:61

@jmjaffe37
Copy link
Contributor Author

Just did a test run using wix 5.0.0 and it seems to have built properly:
https://github.com/jmjaffe37/openjdk-installer/actions/runs/8635971450

I also did an internal test run to create Microsoft OpenJDK MSIs and the resulting MSIs passed the test outlined above (install, correct java version, uninstall as expected)

@jmjaffe37
Copy link
Contributor Author

jmjaffe37 commented Apr 10, 2024

EDIT: I figured out how to test different languages. Below shows what I did:

I have confirmed that all available translations (uncommented lines in /wix/Lang/LanguageList.config) are available and work as expected. Method used to test (run on powershell):
msiexec.exe /i 'C:\<path_to_msi>\OpenJDK17-jdk_x64_windows_hotspot-17.0.10.0.7.msi' ProductLanguage="<LANGUAGE_CODE>"

This is true for MSIs created with wixV4 and wixV5

@karianna
Copy link
Contributor

I think some useful testing will be to upgrade from older installations of Eclipse Temurin and MSFT builds of OpenJDK respectively.

@karianna
Copy link
Contributor

Is anyone able to help me test if the installer translations into other languages/cultures is still behaving as expected? I am unsure on how to do that at the moment 😅

@douph1 - Are you able to check for French?

For the rest I recommend adding a note to the installer channel in Slack and giving folks some instructions on how to self serve this PR and test translations.

@jmjaffe37
Copy link
Contributor Author

Is anyone able to help me test if the installer translations into other languages/cultures is still behaving as expected? I am unsure on how to do that at the moment 😅

@douph1 - Are you able to check for French?

For the rest I recommend adding a note to the installer channel in Slack and giving folks some instructions on how to self serve this PR and test translations.

@karianna I actually figured out how to do it after posting that comment 😅
I updated it to instead explain how I tested it (I made that update before I saw your reply)

@jmjaffe37
Copy link
Contributor Author

Another note: wix v5 was officially release (taken out of beta) on april 5th according to this blog post on their website:
https://www.firegiant.com/blog/2024/4/5/wix-v5.0.0-has-been-released/

Since I did my testing on both wix v4.0.5 and wix v5.0.0 (the first non-beta version), would we like me to change the default wix version to 5.0.0?

@karianna
Copy link
Contributor

Another note: wix v5 was officially release (taken out of beta) on april 5th according to this blog post on their website: https://www.firegiant.com/blog/2024/4/5/wix-v5.0.0-has-been-released/

Since I did my testing on both wix v4.0.5 and wix v5.0.0 (the first non-beta version), would we like me to change the default wix version to 5.0.0?

Yes please

@jmjaffe37 jmjaffe37 changed the title Updated msi windows installers to use wix v4 Updated msi windows installers to use wix v5 Apr 10, 2024
@gdams gdams requested a review from douph1 April 12, 2024 14:38
@jmjaffe37
Copy link
Contributor Author

jmjaffe37 commented Apr 30, 2024

Note: to address a request made here: #888, I implemented a feature to allow vendors to implement custom licenses for hotspot JVMs. It also allows the vendors to choose whether or not to have the end user read and agree to this license (for example, this is optional for the GPLv2 license). Lastly, I added an explanation of this to the README.md

Successful test run with visible GPLv2 license for Eclipse Adoptium vendor: https://github.com/adoptium/installer/actions/runs/8911988475?pr=851

See the PR check for a successful test run for Eclipse Adoptium vendor MSIs without the license showing (default behavior for all vendors with the GPLv2 license unless specified)

Edit: looks like it was decided to remove this logic. The updates have3 been made to remove it properly.

@gdams
Copy link
Member

gdams commented May 2, 2024

raised adoptium/infrastructure#3545 to install wix v5.0.0 on the CI machines

@jmjaffe37
Copy link
Contributor Author

Ok, I believe that this PR is ready to be merged now!

CC: @gdams, @karianna, @douph1

@jmjaffe37 jmjaffe37 requested review from gdams and karianna May 2, 2024 17:57
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.

Some nitpicks

<RegistryValue Root="HKLM"
Key="SOFTWARE\JavaSoft\$(var.OracleJavasoftBaseKey)\$(var.OracleVersionKey)"
Name="RuntimeLib" Type="string" Value="[INSTALLDIR]$(var.DllPath)" KeyPath="no" />
<!-- Oracle doesn't set RuntimeLib for JDK .. because JRE is "private" JRE so no ?else? -->
<?endif?>
<RegistryValue Root="HKLM"
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace for this doesn't match your change above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karianna I am unsure which section you are referring to about the whitespace mismatch. I will say, some of the whitespace tweaks were done by @gdams, but I wanted to change back the inner blocks of if-statements to be indented so the reader is more clear on which lines fall within the if-statement block

Copy link
Member

Choose a reason for hiding this comment

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

@jmjaffe37 feel free to tweak them to however you prefer

.github/workflows/wix.yml Outdated Show resolved Hide resolved
wix/InstallerChangelog.md Show resolved Hide resolved
wix/README.md Outdated Show resolved Hide resolved
@karianna
Copy link
Contributor

karianna commented May 4, 2024

Probably worth communicating this major change to the whole community and landing all of the related PRs in sync. Then we should ask for volunteers to test various scenarios (fresh install, upgrade from previously installed version with old WIX installer, downgrade etc).

Copy link
Contributor

@steelhead31 steelhead31 left a comment

Choose a reason for hiding this comment

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

Tested GHA built MSI installers for JDK8, JDK11, JDK17 & JDK21 on Windows x64, all behave as expected, other than an expected oddity with upgrade behaviour, which will work normally when this PR is merged. Approving on that basis.

@gdams gdams merged commit c660929 into adoptium:master May 9, 2024
9 checks passed
@jmjaffe37 jmjaffe37 deleted the jmj/wix4_update branch May 9, 2024 16:05
@douph1
Copy link
Contributor

douph1 commented Jun 3, 2024

Hi @jmjaffe37 great work.
I have seen you have made best effort to maintain dual capability of installer to work perUser or perMachine.

I have added perUser installer capability in #107

Some time later, Adoptium team have decided that perUser is not the way to go and won't be supported anymore:
See
https://adoptium.net/installation/windows/

The installer is designed for use on a per-machine basis, not per-user basis, so you can have only one installation of the MSI on a machine for all users.

If ALLUSERS was here it was only because cleanup was not done by the dev who disable perUser capability..

@jmjaffe37
Copy link
Contributor Author

jmjaffe37 commented Jun 3, 2024

Hi @jmjaffe37 great work. I have seen you have made best effort to maintain dual capability of installer to work perUser or perMachine.

I have added perUser installer capability in #107

Some time later, Adoptium team have decided that perUser is not the way to go and won't be supported anymore: See https://adoptium.net/installation/windows/

The installer is designed for use on a per-machine basis, not per-user basis, so you can have only one installation of the MSI on a machine for all users.

If ALLUSERS was here it was only because cleanup was not done by the dev who disable perUser capability..

Thanks for letting me know @douph1! This raises a few questions for me:

  1. Does this mean that additional work would be necessary to support a perUser installation? From my understanding, my update takes care of the necessary prerequisites to support this.

  2. Do we prefer to turn off the perUser option?

  3. I noticed that the linked page states that only windows x64 is supported, but Microsoft has been using this code to generate installers for x86 and aarm64 without issue. In fact, recent updates have specifically debugged aarm64 installer creation. Is there somewhere I can propose new wording?

Thanks!

CC: @d3r3kk, @gdams, @karianna

@karianna
Copy link
Contributor

karianna commented Jun 3, 2024

@jmjaffe37 For that last point (docs) - you can send a PR into the adoptium.net repo.

@douph1
Copy link
Contributor

douph1 commented Jun 21, 2024

  1. I think TSC don't want to support "perUser", I don't understand why, as it worked by the past. If changing we probably need a little more testing for install/upgrade/uninstall and maybe cohabitation with x64. I have no time for this.

    1. I have no opinion, nor interest, but thanks for asking :)
  2. Adoptium seems to not build x86 MSI installer after JDK 19. JDK 20/21/22 installer for x86 is missing. I don't know why but it is maybe the meaning of "Note: Windows installer packages are supported only on Windows x64 systems."
    If someone has clue on this we can fix the docs of the website.

Adoptium don't build/test/publish arm64 (AArch64) MSI installer. So this is not documented on the Website, but you can add some wording on wix/README.md

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

Successfully merging this pull request may close these issues.

Generated MSIs lack end-user license agreement Upgrade Wix installers to use V4
5 participants