Skip to content

Experiments can be run successfully with -XX options on older Gradle versions #361

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

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

erichaagdev
Copy link
Member

Context

What is the issue?

Given a Gradle version older than 7.1, when -XX:+UseParallelGC is included as a JVM argument to the Gradle daemon, then user provided system properties from the command line are not available in certain parts of an Initialization script.

Who is impacted?

Any projects which are on a version of Gradle older than 7.1 and are supplying the JVM argument -XX:+UseParallelGC to the Gradle daemon. This can be done by setting the org.gradle.jvmargs property in gradle.properties.

org.gradle.jvmargs="-XX:+UseParallelGC"

What changed between 2.2.1 and 2.3 to cause this regression?

In 2.3, it was decided to migrate all initialization script configuration to system properties. Previously, the configuration was a mix of system properties and project properties. The change was made to be consistent with the TeamCity initialization scripts, with itself, and also be compatible with the org.gradlex.build-parameters plugin which fails the build on unknown project properties.

Is it only system properties that are unavailable?

Yes, but only when accessed via System.getProperty(..). User provided system properties are still accessible via gradle.startParameter.systemPropertiesArgs.get(..). Environment variables accessed via System.getenv(..) and project properties accessed via gradle.startParameter.projectProperties.get(..) are also available.

Is there any other important context to understand for reviewers?

I was unable to find a Gradle Build Tool bug report for this issue. Considering that this issue is not present in Gradle 7.1, it is likely to have been fixed by this change, possibly unintentionally: gradle/gradle#16757

Investigation

I put together a reproducer project to help understand affected Gradle versions and to discover what exactly is available and where. Shown below is a summary of my findings.

3.0 to 8.0.2 System.getProperty(..) systemPropertiesArgs.get(..) System.getenv(..) projectProperties.get(..)
initscript { } ✔️ ✔️ ✔️ ✔️
script root ✔️ ✔️ ✔️ ✔️
settingsEvaluated { } ✔️ ✔️ ✔️ ✔️
3.0 to 7.0.2
XX:+UseParallelGC
System.getProperty(..) systemPropertiesArgs.get(..) System.getenv(..) projectProperties.get(..)
initscript { } ❌️ ✔️ ✔️ ✔️
script root ❌️ ✔️ ✔️ ✔️
settingsEvaluated { } ✔️ ✔️ ✔️ ✔️
7.1 to 8.0.2
XX:+UseParallelGC
System.getProperty(..) systemPropertiesArgs.get(..) System.getenv(..) projectProperties.get(..)
initscript { } ✔️ ✔️ ✔️ ✔️
script root ✔️ ✔️ ✔️ ✔️
settingsEvaluated { } ✔️ ✔️ ✔️ ✔️

Solution

As a solution, I have chosen to replace all usages of System.getProperty(..) with gradle.startParameter.systemPropertiesArgs[name] because it requires very minimal changes to the initialization scripts and no changes to the validation shell scripts. It also will not break the org.gradlex.build-parameters plugin.

It should be noted that this works because these system properties only exist to make the initialization scripts configurable from the shell scripts via command line arguments. This would not work for reading system properties in all use cases (e.g. reading system properties from gradle.properties). This fix will work for the build validation scripts use case, but we should be cautious about where else we apply it.

@erichaagdev erichaagdev self-assigned this Mar 16, 2023
Copy link
Member

@bigdaz bigdaz left a comment

Choose a reason for hiding this comment

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

LGTM

@etiennestuder
Copy link
Member

Is it really related to -XX:+UseParallelGC or does it happen with any kind of -XX options?

@etiennestuder etiennestuder merged commit b6fe7df into main Mar 17, 2023
@etiennestuder etiennestuder deleted the erichaagdev/null-sys-props branch March 17, 2023 07:03
etiennestuder added a commit that referenced this pull request Mar 17, 2023
…er.systemPropertiesArgs in initialization scripts (#361)"

This reverts commit b6fe7df.
@jthurne
Copy link
Member

jthurne commented Mar 17, 2023

We should probably add a [FIX] entry to the release/changes.md for this.

@etiennestuder etiennestuder changed the title Experiments can be run with parallel GC enabled on older Gradle versions Experiments can be run successfully with -XX on older Gradle versions Mar 20, 2023
@etiennestuder etiennestuder changed the title Experiments can be run successfully with -XX on older Gradle versions Experiments can be run successfully with -XX options on older Gradle versions Mar 20, 2023
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.

4 participants