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

Enable 5 version string components instead of 4 #258

Conversation

adamfarley
Copy link
Contributor

@adamfarley adamfarley commented Nov 6, 2020

Previously, version strings were major.minor.maintenance.patch,
where patch was typically the build number.

Now, however, we have a fourth element (also named "patch")
slotted in right before the existing patch element.

So, we're doing four things here:

  1. Handling the new patch variable
  2. Taking this opportunity to adjust the existing patch variable, in
    order to (a) prevent the existence of two patch variables, and (b)
    rename the existing patch variable to "build number", which (since it
    is the build number) seems more appropriate.
  3. Allow upstream to pass in an appropriately-shortened version
    string that wix can use, as anything more than four numbers causes
    an error.
  4. Tidying the litany of IFs in the cmd file.

Signed-off-by: Adam Farley adfarley@redhat.com

@adamfarley
Copy link
Contributor Author

adamfarley commented Nov 6, 2020

Dependant on adoptium/temurin-build#2218
Also adoptium/temurin-build#2219

Advise merging all three around the same time.

Testing this PR here: https://ci.adoptopenjdk.net/view/work-in-progress/job/SXA-create_installer_windows/2/

(Edit: Success! Seeking review and approval on Slack.)

Note: Turns out WIX won't build an installer based on a version string with more than four elements. So for now I'm shedding the +[0-9]* build number on the end.

This will need to be discussed prior to merge, to find out if the build number is really needed in the installer.

wix/Build.OpenJDK_generic.cmd Outdated Show resolved Hide resolved
wix/Build.OpenJDK_generic.cmd Show resolved Hide resolved
@adamfarley adamfarley force-pushed the add_seperate_build_and_patch_numbers branch 2 times, most recently from ef2f1fc to c2dc24e Compare November 9, 2020 18:09
@adamfarley adamfarley force-pushed the add_seperate_build_and_patch_numbers branch 2 times, most recently from bdc243d to 652c16e Compare November 10, 2020 10:37
@adamfarley adamfarley changed the title WIP: Enable 5 version string components instead of 4 Enable 5 version string components instead of 4 Nov 10, 2020
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.

LGTM

wix/Includes/OpenJDK.Variables.wxi Outdated Show resolved Hide resolved
wix/Build.OpenJDK_generic.cmd Outdated Show resolved Hide resolved
wix/Build.OpenJDK_generic.cmd Outdated Show resolved Hide resolved
wix/Build.OpenJDK_generic.cmd Outdated Show resolved Hide resolved
wix/Build.OpenJDK_generic.cmd Outdated Show resolved Hide resolved
wix/Includes/OpenJDK.Variables.wxi Show resolved Hide resolved
@@ -67,7 +71,7 @@ REM

REM Cultures: https://msdn.microsoft.com/de-de/library/ee825488(v=cs.20).aspx
SET PRODUCT_SKU=OpenJDK
SET PRODUCT_VERSION=%PRODUCT_MAJOR_VERSION%.%PRODUCT_MINOR_VERSION%.%PRODUCT_MAINTENANCE_VERSION%.%PRODUCT_PATCH_VERSION%
SET PRODUCT_VERSION=%PRODUCT_MAJOR_VERSION%.%PRODUCT_MINOR_VERSION%.%PRODUCT_MAINTENANCE_VERSION%.%PRODUCT_PATCH_VERSION%.%PRODUCT_BUILD_NUMBER%
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we must add a "+" or a "b" as "build" here before the PRODUCT_BUILD_NUMBER to stay consitant ..
I think Windows can handle "+" in filename and PRODUCT_VERSION is now only used in msi file name OUTPUT_BASE_FILENAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR, the "PRODUCT_PATCH_VERSION" is actually the build number. It's named wrong, and this PR fixes that.
Adding a plus sign or a "b" is an additional behaviour change.

@adamfarley adamfarley force-pushed the add_seperate_build_and_patch_numbers branch from 652c16e to 7b89d72 Compare November 10, 2020 12:21
Previously, version strings were major.minor.maintenance.patch,
where patch was typically the build number.

Now, however, we have a fourth element (also named "patch")
slotted in right before the existing patch element.

So, we're doing four things here:

1) Handling the new patch variable
2) Taking this opportunity to adjust the existing patch variable, in
order to (a) prevent the existence of two patch variables, and (b)
rename the existing patch variable to "build number", which (since it
*is* the build number) seems more appropriate.
3) Allow upstream to pass in an appropriately-shortened version
string that wix can use, as anything more than four numbers causes
an error.
4) Tidying the litany of IFs in the cmd file.

Signed-off-by: Adam Farley <adfarley@redhat.com>
@adamfarley adamfarley force-pushed the add_seperate_build_and_patch_numbers branch from 7b89d72 to 247dae0 Compare November 10, 2020 12:22
@andrew-m-leonard andrew-m-leonard merged commit fac72c1 into adoptium:master Nov 10, 2020
@adamfarley adamfarley mentioned this pull request Nov 11, 2020
@adamfarley adamfarley deleted the add_seperate_build_and_patch_numbers branch July 25, 2023 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants