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

Upgrade Gradle to 8.6, use Java 17 for building code, target Java 11, polish buildscripts #3067

Merged
merged 5 commits into from Feb 15, 2024

Conversation

vlsi
Copy link
Contributor

@vlsi vlsi commented Feb 15, 2024

Did you remember to?

  • Add test case(s)
  • Update CHANGES.txt
  • Auto applied styling via ./gradlew autostyleApply

Summary

  • The generated code would use Java 11 bytecode (the same as the current TestNG)
  • The scripts will use Java 17 (a recent LTS) for building classes to reduce the possibility for javac errors. In theory, we could use Java 21 for the build (and still target 11), however, it might be too recent
  • build-logic/build-parameters/build.gradle.kts declares the build parameters like targetJavaVersion, jdkBuildVersion, jdkTestVersion, and so on. See ./gradlew parameters for the full list
  • Now we test with Java EA versions like Java 22. The EA version is coded in .github/workflows/matrix.js, see eaJava

Summary by CodeRabbit

  • New Features
    • Introduced new Java version and distribution axes in CI workflows for more comprehensive testing.
    • Added Gradle plugin configurations and Kotlin DSL for improved build logic.
    • Implemented Java toolchain configuration for specifying JDK versions and vendors.
    • Enhanced CI scripts with concurrency settings, Java setup modifications, and reproducibility scripts.
    • Updated Gradle wrapper properties for better distribution management.
  • Bug Fixes
    • Corrected typo from totalWeigth to totalWeight in matrix builder logic.
    • Replaced loose equality checks with strict equality in various scripts.
  • Refactor
    • Updated build logic across various modules for cleaner dependency declarations and plugin usage.
    • Improved error handling and filtering in CI matrix generation.
  • Documentation
    • Enhanced comments in gradlew script regarding shell compatibility and OS-specific tweaks.
  • Chores
    • Updated .gitignore patterns for build directories.
    • Removed and added system properties in gradle.properties for optimized build performance.

Copy link

coderabbitai bot commented Feb 15, 2024

Warning

Rate Limit Exceeded

@vlsi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 26 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between d6ed528 and 9f0406d.

Walkthrough

These updates focus on refining the Java development environment by renaming axes, adding Java versions, and enhancing Gradle configurations. Improvements include stricter code quality checks, better error handling, and updates to the build logic. The changes aim to optimize Java and Gradle setups for more efficient and error-free builds, reflecting a concerted effort to streamline development workflows and toolchain management.

Changes

Files Change Summary
.github/workflows/matrix.js Renamed axes, added java_version, updated job generation, added Gradle and JVM arguments.
.github/workflows/matrix_builder.js Fixed typos, enhanced logic with implications and error handling, added summary method.
.github/workflows/test.yml Added concurrency settings, modified Java setup steps, added script for CI results.
build-logic-commons/.../.gitignore Added ignore patterns for build directories.
build-logic-commons/.../build.gradle.kts Set up Gradle plugin using Kotlin DSL, configured Java version compatibility.
build-logic/..., build-logic/jvm/... Updated plugin IDs, dependencies, removed KotlinCompile configurations, added toolchain config.
gradle.properties, gradlew, gradlew.bat Updated Gradle wrapper properties, improved script robustness and error handling.
settings.gradle.kts Added plugins, enabled feature previews, included projects.

"In the realm of code, where Java beans grow,
A rabbit hopped in, with updates in tow.
🐇 With each leap and bound, the build scripts refine,
Ensuring that Java and Gradle align.
So cheers to the changes, both big and small,
For they make our builds smoother, once and for all."

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 12

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between d6ed528 and 02c066a.
Files ignored due to path filters (1)
  • gradle/wrapper/gradle-wrapper.jar is excluded by: !**/*.jar
Files selected for processing (27)
  • .github/workflows/matrix.js (4 hunks)
  • .github/workflows/matrix_builder.js (10 hunks)
  • .github/workflows/test.yml (3 hunks)
  • build-logic-commons/.gitignore (1 hunks)
  • build-logic-commons/gradle-plugin/build.gradle.kts (1 hunks)
  • build-logic-commons/gradle-plugin/src/main/kotlin/build-logic.kotlin-dsl-gradle-plugin.gradle.kts (1 hunks)
  • build-logic-commons/settings.gradle.kts (1 hunks)
  • build-logic/basics/build.gradle.kts (1 hunks)
  • build-logic/basics/src/main/kotlin/ToolchainProperties.kt (1 hunks)
  • build-logic/basics/src/main/kotlin/configureToolchain.kt (1 hunks)
  • build-logic/build-parameters/build.gradle.kts (1 hunks)
  • build-logic/build.gradle.kts (1 hunks)
  • build-logic/code-quality/build.gradle.kts (1 hunks)
  • build-logic/code-quality/src/main/kotlin/testng.testing.gradle.kts (2 hunks)
  • build-logic/jvm/build.gradle.kts (1 hunks)
  • build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (2 hunks)
  • build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts (1 hunks)
  • build-logic/publishing/build.gradle.kts (1 hunks)
  • build-logic/publishing/src/main/kotlin/buildlogic/FirstLayerDependencies.kt (2 hunks)
  • build-logic/settings.gradle.kts (1 hunks)
  • build.gradle.kts (1 hunks)
  • gradle.properties (2 hunks)
  • gradle/wrapper/gradle-wrapper.properties (1 hunks)
  • gradlew (4 hunks)
  • gradlew.bat (4 hunks)
  • settings.gradle.kts (2 hunks)
  • testng-test-osgi/testng-test-osgi-build.gradle.kts (2 hunks)
Files skipped from review due to trivial changes (1)
  • build-logic-commons/.gitignore
Additional comments: 52
build-logic/build.gradle.kts (1)
  • 1-3: The embedded-kotlin plugin is applied with apply false, which is correct for a build script that may depend on Kotlin for build logic but does not directly apply the plugin to the project.
build-logic/basics/build.gradle.kts (2)
  • 2-2: The plugin ID has been correctly updated to id("build-logic.kotlin-dsl-gradle-plugin"), aligning with the project's naming conventions and structure.
  • 6-6: The dependency api(projects.buildParameters) is added, ensuring that the buildParameters project is correctly referenced and its artifacts are available to this project.
build-logic-commons/settings.gradle.kts (3)
  • 1-5: The dependencyResolutionManagement block with gradlePluginPortal() repository is correctly configured, ensuring that Gradle plugins can be resolved from the official plugin portal.
  • 7-7: Setting the root project name to build-logic-commons is appropriate for identifying the project within this build.
  • 9-9: Including the gradle-plugin project is correct, ensuring it's part of the build.
build-logic/code-quality/build.gradle.kts (2)
  • 2-2: The plugin ID id("build-logic.kotlin-dsl-gradle-plugin") is correctly applied, consistent with the project's structure.
  • 6-9: Dependencies on projects.buildParameters, projects.basics, and external libraries (org.sonarqube and com.github.autostyle) are correctly declared using api, ensuring they are available to consumers of this project.
build-logic/settings.gradle.kts (2)
  • 9-9: Enabling the feature preview TYPESAFE_PROJECT_ACCESSORS is a good practice for type-safe access to project properties.
  • 11-13: Including builds and projects such as build-logic-commons and build-parameters is correctly done, ensuring these components are part of the build.
gradle/wrapper/gradle-wrapper.properties (1)
  • 2-6: The update to gradle-wrapper.properties with new properties like distributionPath, distributionSha256Sum, distributionUrl, networkTimeout, and validateDistributionUrl is correctly done, enhancing the security and configuration of the Gradle Wrapper.
build-logic/publishing/build.gradle.kts (2)
  • 2-2: The plugin ID id("build-logic.kotlin-dsl-gradle-plugin") is correctly applied, aligning with the project's structure.
  • 6-9: Dependencies on projects.jvm and external libraries (com.github.vlsi.gradle-extensions, com.github.johnrengelman.shadow, org.jetbrains.kotlin:kotlin-gradle-plugin) are correctly declared using api, ensuring they are available to consumers of this project.
build-logic/jvm/build.gradle.kts (2)
  • 2-2: The plugin ID id("build-logic.kotlin-dsl-gradle-plugin") is correctly applied, consistent with the project's structure.
  • 6-10: Dependencies on projects.buildParameters, projects.basics, projects.codeQuality, and external libraries (com.github.vlsi.gradle-extensions, org.jetbrains.kotlin.jvm) are correctly declared using api, ensuring they are available to consumers of this project.
build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts (1)
  • 15-18: The addition of compiler arguments for specifying JVM defaults and JDK release based on the target Java version is correctly implemented, ensuring compatibility and leveraging new language features.
build-logic-commons/gradle-plugin/src/main/kotlin/build-logic.kotlin-dsl-gradle-plugin.gradle.kts (3)
  • 1-4: The plugin declarations for java-library and org.gradle.kotlin.kotlin-dsl are correctly configured, setting up the necessary environment for this Gradle plugin project.
  • 6-9: Enabling stricter validation for plugin validation tasks is a good practice to ensure quality and compatibility of the plugins developed.
  • 11-21: The logic to determine the JVM version (17 or 11) based on the current JVM's capabilities is correctly implemented, ensuring compatibility with the build environment.
build-logic/basics/src/main/kotlin/ToolchainProperties.kt (2)
  • 1-8: The class ToolchainProperties and its properties are correctly defined, providing a structured way to handle toolchain configurations.
  • 10-20: Extension properties for BuildParametersExtension to derive toolchain properties for build and test JDKs are correctly implemented, facilitating easy access to these configurations.
gradle.properties (1)
  • 24-26: Adding systemProp.sonar.gradle.skipCompile=true is correctly done to address Gradle warning with SonarQube plugin, following best practices for plugin configuration.
settings.gradle.kts (1)
  • 10-10: Adding the plugin org.gradle.toolchains.foojay-resolver-convention with version 0.8.0 is correctly done, enhancing the project's toolchain resolution capabilities.
build-logic/basics/src/main/kotlin/configureToolchain.kt (1)
  • 9-24: The functions launcherFor and configureToolchain are correctly implemented, providing a structured way to configure the Java toolchain based on properties defined in ToolchainProperties.
build-logic-commons/gradle-plugin/build.gradle.kts (2)
  • 1-15: The plugin kotlin-dsl is correctly applied, and the dependency on org.gradle.kotlin.kotlin-dsl:org.gradle.kotlin.kotlin-dsl.gradle.plugin is correctly declared, ensuring the necessary components for precompiled script plugins are available.
  • 17-27: The logic to determine the JVM version (17 or 11) based on the current JVM's capabilities is correctly implemented, ensuring compatibility with the build environment.
build-logic/code-quality/src/main/kotlin/testng.testing.gradle.kts (2)
  • 5-5: The addition of the build-logic.build-params plugin is correctly done, ensuring that build parameters are correctly configured for the project.
  • 14-16: Configuring the javaLauncher based on buildParameters.testJdk is correctly implemented, ensuring that tests run with the specified JDK version.
build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (4)
  • 5-5: Adding the build-logic.build-params plugin is correctly done, ensuring that build parameters are correctly configured for the project.
  • 15-17: Configuring the toolchain with buildParameters.buildJdk is correctly implemented, ensuring that the build uses the specified JDK version.
  • 20-24: Setting the options.release for JavaCompile tasks based on buildParameters.targetJavaVersion is correctly done, ensuring compatibility with the target Java version.
  • 26-29: Configuring the javaLauncher for JavaExec tasks based on buildParameters.testJdk is correctly implemented, ensuring that Java applications run with the specified JDK version.
build.gradle.kts (1)
  • 13-17: Adding a new task parameters that displays supported build parameters and depends on a task from build-logic is correctly implemented, enhancing the project's build documentation and usability.
build-logic/build-parameters/build.gradle.kts (2)
  • 1-5: The plugins org.gradlex.build-parameters, com.github.vlsi.gradle-extensions, and build-logic.kotlin-dsl-gradle-plugin are correctly applied, setting up the necessary environment for managing build parameters.
  • 7-51: The configuration of build parameters, including boolean, integer, and string types, is correctly done, providing a comprehensive and flexible way to manage build configurations.
gradlew.bat (4)
  • 17-17: The conditional check for %DEBUG% is correctly implemented, allowing for optional debug output.
  • 28-29: Setting DIRNAME to the current directory if it's empty is correctly done, ensuring the script can locate its resources.
  • 44-44: The error handling for when java.exe cannot be found in the PATH is correctly implemented, providing clear instructions to the user.
  • 79-87: Setting EXIT_CODE based on the error level and handling GRADLE_EXIT_CONSOLE is correctly done, ensuring proper script termination behavior.
testng-test-osgi/testng-test-osgi-build.gradle.kts (1)
  • 38-44: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [41-52]

Updating the assignment of the type property within the generateDependenciesProperties task to handle it.extension being null is correctly done, enhancing robustness.

build-logic/publishing/src/main/kotlin/buildlogic/FirstLayerDependencies.kt (1)
  • 61-62: Modifying how usageKind is calculated by using replaceFirstChar instead of capitalization logic is correctly done, improving the clarity and correctness of the code.
.github/workflows/test.yml (4)
  • 13-18: Adding concurrency settings for job cancellation based on branch type is correctly done, optimizing CI resource usage.
  • 50-56: Modifying Java setup steps to handle different Java versions and distributions is correctly implemented, ensuring compatibility across various Java environments.
  • 65-70: Adding a script for reproducing CI results is a good practice, enhancing the transparency and reproducibility of CI builds.
  • 82-90: Enhancing build properties with JDK versions and vendors is correctly done, providing more control and flexibility over the build environment.
.github/workflows/matrix.js (1)
  • 72-74: The exclusion of Oracle JDK for Java version 11 might need reevaluation or additional comments for clarity, especially considering Oracle's licensing and distribution changes over time. Ensure this logic aligns with current Oracle JDK policies for Java 11.
.github/workflows/matrix_builder.js (3)
  • 12-12: Corrected typo from totalWeigth to totalWeight improves readability and consistency. Good fix.
  • 21-21: Changing find to some in the matches method is a logical improvement for performance, as it stops iterating once a match is found. Good optimization.
  • 32-32: Using strict equality checks (===) instead of loose equality (==) is a best practice in JavaScript to avoid unexpected type coercion. Good fix.
gradlew (3)
  • 25-39: The detailed comments about POSIX-compliant shells and shell features required are informative and help clarify the script's requirements. This is good documentation practice.
  • 54-55: The mention of tweaks for specific operating systems (AIX, CygWin, Darwin, MinGW, NonStop) is important for understanding the script's compatibility. Ensure that these tweaks are kept up-to-date with the latest OS versions and peculiarities.
  • 68-80: The logic for resolving symlinks is robust and accounts for various symlink scenarios. This is crucial for ensuring the script works correctly in environments where symlinks are common.

Comment on lines +23 to +31
const eaJava = '22';
matrix.addAxis({
name: 'java_version',
// Strings allow versions like 18-ea
values: [
'11',
'17',
'21',
eaJava,
Copy link

Choose a reason for hiding this comment

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

The explanation about POSIX-compliant shells and the example provided using ksh Gradle seems to be a copy-paste error from a Gradle script. This is misleading in the context of a JavaScript file intended to be run with Node.js.

- #           ksh Gradle
+ #           node matrix.js

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const eaJava = '22';
matrix.addAxis({
name: 'java_version',
// Strings allow versions like 18-ea
values: [
'11',
'17',
'21',
eaJava,
const eaJava = '22';
matrix.addAxis({
name: 'java_version',
// Strings allow versions like 18-ea
values: [
'11',
'17',
'21',
eaJava,

{value: 'oracle', vendor: 'oracle', weight: 1},
// There are issues running Semeru JDK with Gradle 8.5
// See https://github.com/gradle/gradle/issues/27273
// {value: 'semeru', vendor: 'ibm', weight: 4},
Copy link

Choose a reason for hiding this comment

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

Commented-out code related to the semeru Java distribution is present. If this is intentionally left for future reference, consider adding a more detailed comment explaining the circumstances under which it might be uncommented. Otherwise, remove it to clean up the code.

Comment on lines +96 to +104
include.forEach(v => {
// Pass locale via Gradle arguments in case it won't be inherited from _JAVA_OPTIONS
// In fact, _JAVA_OPTIONS is non-standard and might be ignored by some JVMs
let gradleArgs = [
`-Duser.country=${v.locale.country}`,
`-Duser.language=${v.locale.language}`,
];
v.extraGradleArgs = gradleArgs.join(' ');
});
Copy link

Choose a reason for hiding this comment

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

Passing locale settings via Gradle arguments is a good practice for ensuring consistent builds across environments. However, ensure that these settings are indeed required for all jobs and consider if there might be a more centralized way to apply these settings to avoid repetition.

Comment on lines +113 to +136
v.java_vendor = v.java_distribution.vendor;
v.java_distribution = v.java_distribution.value;
if (v.java_distribution === 'oracle') {
v.oracle_java_website = v.java_version === eaJava ? 'jdk.java.net' : 'oracle.com';
}
v.non_ea_java_version = v.java_version === eaJava ? '' : v.java_version;
if (v.java_distribution !== 'semeru' && Math.random() > 0.5) {
// The following options randomize instruction selection in JIT compiler
// so it might reveal missing synchronization in TestNG code
v.name += ', stress JIT';
v.testDisableCaching = 'JIT randomization should not be cached';
jvmArgs.push('-XX:+UnlockDiagnosticVMOptions');
// Randomize instruction scheduling in GCM
// share/opto/c2_globals.hpp
jvmArgs.push('-XX:+StressGCM');
// Randomize instruction scheduling in LCM
// share/opto/c2_globals.hpp
jvmArgs.push('-XX:+StressLCM');
if (v.jdk.version >= 16) {
if (v.java_version >= 16) {
// Randomize worklist traversal in IGVN
// share/opto/c2_globals.hpp
jvmArgs.push('-XX:+StressIGVN');
}
if (v.jdk.version >= 17) {
if (v.java_version >= 17) {
Copy link

Choose a reason for hiding this comment

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

The logic for adding JVM arguments based on the hash value and Java version checks for enabling JIT randomization is complex and could benefit from a more modular approach. Consider refactoring this into a separate function or method to improve readability and maintainability.

Comment on lines +147 to +152
let filePath = process.env['GITHUB_OUTPUT'] || '';
if (filePath) {
fs.appendFileSync(filePath, `matrix<<MATRIX_BODY${os.EOL}${JSON.stringify({include})}${os.EOL}MATRIX_BODY${os.EOL}`, {
encoding: 'utf8'
});
}
Copy link

Choose a reason for hiding this comment

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

The use of fs.appendFileSync within a conditional block based on filePath being truthy is correct. However, ensure error handling is in place for file system operations to gracefully handle potential write failures.

Comment on lines +180 to +186
const filterStr = typeof filter === 'string' ? filter.toString() : JSON.stringify(filter);
const msg = `Unable to generate row for ${filterStr}. Please check include and exclude filters`;
if (this.failOnUnsatisfiableFilters) {
throw Error(msg);
} else {
console.warn(msg);
}
Copy link

Choose a reason for hiding this comment

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

The error handling for the generateRow method when unable to generate a row for a given filter is improved by the option to throw an error or warn based on failOnUnsatisfiableFilters. This adds flexibility but ensure that this behavior is clearly documented for users of the class.

Comment on lines +200 to +252
summary() {
let position = -1;
let indices = [];
let values = {};
const axes = this.axes;
function resetValuesUpTo(nextPosition) {
for(let i=0; i<nextPosition; i++) {
const axis = axes[i];
values[axis.name] = axis.values[0];
indices[i] = 1; // next index
}
position = 0;
}

function nextAvailablePosition() {
let size = axes.length;
for (let i = position; i < size; i++) {
if (indices[i] < axes[i].values.length) {
return i;
}
}
return -1;
}
// The first initialization of the values
resetValuesUpTo(this.axes.length);
let good = 0;
let bad = 0;
while (true) {
if (indices[position] < this.axes[position].values.length) {
// Advance iterator at the current position if possible
const axis = this.axes[position];
values[axis.name] = axis.values[indices[position]];
indices[position]++;
} else {
// Advance the next iterator, and reset [0..nextPosition)
position++;
let nextPosition = nextAvailablePosition();
if (nextPosition === -1) {
break;
}
const axis = this.axes[nextPosition];
values[axis.name] = axis.values[indices[nextPosition]];
indices[nextPosition]++;
resetValuesUpTo(nextPosition);
}
if (this.matches(values)) {
good++;
} else {
bad++;
}
}
return {good: good, bad: bad};
}
Copy link

Choose a reason for hiding this comment

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

The summary method to compute the number of possible combinations is a valuable addition for debugging and analysis purposes. Ensure that its performance impact is minimal and consider adding caching if the computation is expensive and called frequently.


# Use the maximum available, or set MAX_FD != -1 to use that value.
MAX_FD="maximum"
MAX_FD=maximum
Copy link

Choose a reason for hiding this comment

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

Setting MAX_FD=maximum without further explanation or a configurable option might not be suitable for all environments. Consider adding a comment explaining why this value is chosen and how users can adjust it if necessary.

Comment on lines +171 to +197
if "$cygwin" || "$msys" ; then
APP_HOME=$( cygpath --path --mixed "$APP_HOME" )
CLASSPATH=$( cygpath --path --mixed "$CLASSPATH" )

JAVACMD=$( cygpath --unix "$JAVACMD" )

# Now convert the arguments - kludge to limit ourselves to /bin/sh
i=0
for arg in "$@" ; do
CHECK=`echo "$arg"|egrep -c "$OURCYGPATTERN" -`
CHECK2=`echo "$arg"|egrep -c "^-"` ### Determine if an option

if [ $CHECK -ne 0 ] && [ $CHECK2 -eq 0 ] ; then ### Added a condition
eval `echo args$i`=`cygpath --path --ignore --mixed "$arg"`
else
eval `echo args$i`="\"$arg\""
for arg do
if
case $arg in #(
-*) false ;; # don't mess with options #(
/?*) t=${arg#/} t=/${t%%/*} # looks like a POSIX filepath
[ -e "$t" ] ;; #(
*) false ;;
esac
then
arg=$( cygpath --path --ignore --mixed "$arg" )
fi
i=`expr $i + 1`
# Roll the args list around exactly as many times as the number of
# args, so each arg winds up back in the position where it started, but
# possibly modified.
#
# NB: a `for` loop captures its iteration list before it begins, so
# changing the positional parameters here affects neither the number of
# iterations, nor the values presented in `arg`.
shift # remove old arg
set -- "$@" "$arg" # push replacement arg
Copy link

Choose a reason for hiding this comment

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

The conversion of paths to Windows format for Cygwin or MSYS environments is a necessary compatibility feature. However, the loop for processing arguments is complex and could be prone to errors. Test thoroughly, especially with edge cases like paths containing spaces or special characters.

Comment on lines +242 to +247
eval "set -- $(
printf '%s\n' "$DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS" |
xargs -n1 |
sed ' s~[^-[:alnum:]+,./:=@_]~\\&~g; ' |
tr '\n' ' '
)" '"$@"'
Copy link

Choose a reason for hiding this comment

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

The use of eval to parse JVM options is potentially dangerous as it can execute arbitrary code. Ensure that the inputs to this section are sanitized or consider safer alternatives to eval for parsing and handling JVM options.

- eval "set -- $(
+ # Consider safer parsing alternatives to eval

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
eval "set -- $(
printf '%s\n' "$DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS" |
xargs -n1 |
sed ' s~[^-[:alnum:]+,./:=@_]~\\&~g; ' |
tr '\n' ' '
)" '"$@"'
# Consider safer parsing alternatives to eval "set -- $(
printf '%s\n' "$DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS" |
xargs -n1 |
sed ' s~[^-[:alnum:]+,./:=@_]~\\&~g; ' |
tr '\n' ' '
)" '"$@"'

Previously, TestNG was build with the "current Java" version which was not very reproducible.
Now the build script would use Java 17 for building the code no matter which Java is selected for
launching Gradle.

Even though the code is always build with Java 17, the tests execute with a configurable Java version.
It enables executing tests with EA Java versions.
@juherr
Copy link
Member

juherr commented Feb 15, 2024

@vlsi Many thanks for that! 👍
About the matrix logic, have you ever considere to propose it in a dedicated action where we just configure options and maybe define contraints.

@juherr juherr merged commit 196babc into testng-team:master Feb 15, 2024
6 of 9 checks passed
@vlsi
Copy link
Contributor Author

vlsi commented Feb 15, 2024

just configure options and maybe define contraints.

I find it nice and easy to debug by running node matrix.js locally.
At the same time, factoring the code into an action might take much more time than updating individual files once a year :)

This was referenced Feb 15, 2024
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

2 participants