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

Update Main.wxs.template #842

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

Conversation

adipiciu
Copy link
Contributor

Removed the JavaSoftCurrentVersion component condition.

Temurin 8 JavaSoftCurrentVersion key is not removed at uninstall, because the condition is never satisfied. For Temurin 11, 17, etc, the condition doesn't make any sense because it's comparing with version "1.8" and it's always true, and Temurin 8 has totally different keys anyways.

Removed the JavaSoftCurrentVersion component condition.

Temurin 8 JavaSoftCurrentVersion key is not removed at uninstall, because the condition is never satisfied. For Temurin 11, 17, etc, the condition doesn't make any sense because it's comparing with version "1.8", but Temurin 8 has totally different keys.

Signed-off-by: adipiciu <13155063+adipiciu@users.noreply.github.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

A block has been put on this Pull Request as this repository is temporarily under a code freeze due to an ongoing release cycle.

If this pull request needs to be merged during the release cycle then please comment /merge and a PMC member will be able to remove the block.

If the code freeze is over you can remove this block by commenting /thaw.

@karianna karianna requested review from douph1 and gdams March 16, 2024 21:53
@douph1
Copy link
Contributor

douph1 commented Apr 9, 2024

The condition was added to prevent overriding CurrentVersion with last version over higher version.

As mentioned in comment here :
#329 (comment)

I have fixed the wrong comparaison order "<" in 26d01e8 but not fixed the jdk8 specific case

I think we must handle the 1.8 case not remove the logic completly.
The actual part of the condition "JAVASOFT_CURRENTVERSION <> "1.8" " is probably making Wix to completly ignore
then <RegistryValue ... /> when JDK 1.8 is installed.

#329

For Temurin 11, 17, etc, the condition doesn't make any sense because it's comparing with version "1.8" and it's always true

The condition skip comparing existing installed ( (1.8)left <>) with installer version.

Maybe we must add another test for ignoring (<> right (1.8) ) : $(var.ProductMajorVersion) <> 1.8
or just handle "1.8" CurrentVersion after.

With other than "1.8" version, numeric compare is done to not lower CurrentVersion with older version when you install old java after last version.

Goal :
Install Java 21 > CurrentVersion = 21
Then
Install Java 17 > CurrentVersion = 21

@adipiciu
Copy link
Contributor Author

adipiciu commented Apr 9, 2024

I agree for Temurin 11, 17, etc. Then the condition should be changed to:

NOT JAVASOFT_CURRENTVERSION OR JAVASOFT_CURRENTVERSION <= $(var.OracleVersionKey)

The condition is evaluated both at install and uninstall.

At install -> it will write the registry key if it's null, or if the registry value is the same or lower than the installer version key.
At uninstall -> it will remove the key if it's null, or if the registry value is the same or lower than the installer version key.

So, the logic is that it will remove the key only if the value is same or lower than the temurin installer version (8, 11, 17, etc.)

Btw, the var.OracleVersionKey value is 8, not 1.8. So the condition was wrong from the start.

@karianna
Copy link
Contributor

/thaw

@github-actions github-actions bot dismissed their stale review April 11, 2024 00:12

Pull Request unblocked - code freeze is over.

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.

None yet

3 participants