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.

Sign up for GitHub

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

Jump to bottom

chore(common): Add minimum-versions.inc.sh #11380

Merged
merged 5 commits into from
May 8, 2024
Merged

chore(common): Add minimum-versions.inc.sh #11380

merged 5 commits into from
May 8, 2024

Conversation

ermshiperete
Copy link
Contributor

This file contains the minimal required versions as discussed for Keyman 18.

Implements #11371.

@keymanapp-test-bot skip

@ermshiperete
chore(common): Add minimum-versions.inc.sh
6f7acf8 
This file contains the minimal required versions as discussed for
Keyman 18.

Implements #11371.
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented May 6, 2024 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S1 milestone May 6, 2024
mcdurdin
mcdurdin requested changes May 6, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

This looks good, I have made a bunch of suggestions which are just building on this nice base.

resources/build/minimum-versions.inc.sh Outdated
Comment on lines 7 to 23
MIN_NODE_MAJOR_VERSION=18
MIN_EMSCRIPTEN_VERSION=3.1.44 # 3.1.45 has problems, newer versions will work

MIN_VISUAL_STUDIO_VERSION=2019
MIN_MESON_VERSION=1.0.0
MIN_CHROME_VERSION=95.0

MIN_ANDROID_VERSION=5
MIN_IOS_VERSION=12.2
MIN_WINDOWS_VERSION=10 # Windows 10
MIN_MACOS_VERSION=10.13 # MacOS 10.13 (High Sierra)
MIN_UBUNTU_VERSION=20.04 # Ubuntu 20.04 Focal

# We're using Java/OpenJDK 11
KEYMAN_JAVA_VERSION=11

DEFAULT_UBUNTU_VERSION=noble
Copy link
Member

Choose a reason for hiding this comment

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

I would like to prefix all variables with KEYMAN_MIN_VERSION or similar. It's a bit long but it avoids namespacing issues, particularly if we add other deps here in the future.

There are slightly different meanings and purposes for these variables: OS versions, Chrome version are target versions. The others are pretty much all dep versions.

I think the target min OS Versions section should be first, along with min chrome ver.

I renamed MACOS to MAC to match folder and other variable names (there still some that say macos or macosx but not too many now).

I am not sure what the DEFAULT_UBUNTU_VERSION is for (and it would be nice it could show a version number in the comments for comparison since you used a name here).

Added C++ version, max emscripten version, as extra notes.

Suggested change
MIN_NODE_MAJOR_VERSION=18
MIN_EMSCRIPTEN_VERSION=3.1.44 # 3.1.45 has problems, newer versions will work
MIN_VISUAL_STUDIO_VERSION=2019
MIN_MESON_VERSION=1.0.0
MIN_CHROME_VERSION=95.0
MIN_ANDROID_VERSION=5
MIN_IOS_VERSION=12.2
MIN_WINDOWS_VERSION=10 # Windows 10
MIN_MACOS_VERSION=10.13 # MacOS 10.13 (High Sierra)
MIN_UBUNTU_VERSION=20.04 # Ubuntu 20.04 Focal
# We're using Java/OpenJDK 11
KEYMAN_JAVA_VERSION=11
DEFAULT_UBUNTU_VERSION=noble
# Target operating system and platform versions
KEYMAN_MIN_TARGET_VERSION_ANDROID=5 # Lollipop
KEYMAN_MIN_TARGET_VERSION_IOS=12.2 # iOS 12.2
KEYMAN_MIN_TARGET_VERSION_WINDOWS=10 # Windows 10
KEYMAN_MIN_TARGET_VERSION_MAC=10.13 # MacOS 10.13 (High Sierra)
KEYMAN_MIN_TARGET_VERSION_UBUNTU=20.04 # Ubuntu 20.04 Focal
KEYMAN_MIN_TARGET_VERSION_CHROME=95.0 # Final version that runs on Android 5.0
# Dependency versions
KEYMAN_MIN_VERSION_NODE_MAJOR=18
KEYMAN_MIN_VERSION_NPM=10.5.1 # 10.5.0 has bug, discussed in #10350
KEYMAN_MIN_VERSION_EMSCRIPTEN=3.1.44 # Warning: 3.1.45 is bad (#9529); newer versions work
KEYMAN_MAX_VERSION_EMSCRIPTEN=3.1.58 # See #9529
KEYMAN_MIN_VERSION_VISUAL_STUDIO=2019
KEYMAN_MIN_VERSION_MESON=1.0.0
# Language and runtime versions
KEYMAN_MIN_VERSION_JAVA=11 # We're using Java/OpenJDK 11
KEYMAN_MIN_VERSION_CPP=17 # C++17
KEYMAN_DEFAULT_VERSION_UBUNTU=noble # Ubuntu 24.04 Noble

@mcdurdin mcdurdin linked an issue May 6, 2024 that may be closed by this pull request
chore(common): make /resources/build/minimum-versions.inc.sh #11371
Closed
@mcdurdin
Copy link
Member

mcdurdin commented May 6, 2024

One more thought: this is also the ideal place, in comments, to document reasoning behind min versions. If we put reasoning elsewhere, it will diverge...

@darcywong00 darcywong00 mentioned this pull request May 7, 2024
Closed
@ermshiperete
Copy link
Contributor Author

I am not sure what the DEFAULT_UBUNTU_VERSION is for (and it would be nice it could show a version number in the comments for comparison since you used a name here).

That's bleeding through from an upcoming PR. It's the default Ubuntu version used in Docker containers. I renamed it, maybe that makes it clearer.

@ermshiperete ermshiperete requested a review from mcdurdin May 7, 2024 14:08
ermshiperete
ermshiperete commented May 7, 2024
View reviewed changes
resources/build/minimum-versions.inc.sh Outdated
KEYMAN_MIN_VERSION_MESON=1.0.0

# Language and runtime versions
KEYMAN_MIN_VERSION_JAVA=11 # We're using Java/OpenJDK 11
Copy link
Contributor Author

@ermshiperete ermshiperete May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do builds work with higher Java versions? Otherwise we should rename it:

Suggested change
KEYMAN_MIN_VERSION_JAVA=11 # We're using Java/OpenJDK 11
KEYMAN_VERSION_JAVA=11 # We're using Java/OpenJDK 11

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe? I'm currently investigating changes for Android building with Java 17 or 21.
But maybe some Java versions > 11 just work?

Renaming the variable sounds fine to me.

mcdurdin
mcdurdin reviewed May 8, 2024
View reviewed changes
resources/build/minimum-versions.inc.sh Outdated
KEYMAN_MIN_VERSION_ANDROID_SDK=21

# Default version used in Docker containers
KEYMAN_DEFAULT_VERSION_CONTAINER=noble # Ubuntu 24.04 Noble
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
KEYMAN_DEFAULT_VERSION_CONTAINER=noble # Ubuntu 24.04 Noble
KEYMAN_DEFAULT_VERSION_UBUNTU_CONTAINER=noble # Ubuntu 24.04 Noble

Yeah it's long but it's clearer that we are Ubuntu and not some other distro or OS

@ermshiperete ermshiperete merged commit 0f6a73d into master May 8, 2024
25 checks passed
@ermshiperete ermshiperete deleted the chore/common/minversions.sh branch May 8, 2024 18:16
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.32-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darcywong00 darcywong00 darcywong00 left review comments

@mcdurdin mcdurdin mcdurdin approved these changes

@srl295 srl295 srl295 approved these changes

@jahorton jahorton Awaiting requested review from jahorton jahorton is a code owner

Assignees
No one assigned
Labels
chore common/resources/ Build infrastructure common/
Projects
None yet
Milestone
A18S1
Development

Successfully merging this pull request may close these issues.

chore(common): make /resources/build/minimum-versions.inc.sh
6 participants
@ermshiperete @mcdurdin @keyman-server @srl295 @darcywong00 @jahorton