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

Environment variable changes are reverted and gradle.startParameter is used to read system properties in init scripts #682

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

erichaagdev
Copy link
Member

@erichaagdev erichaagdev commented Nov 20, 2024

This PR:

  1. Reverts the change for configuring init scripts via environment variables
  2. (re)Migrates init scripts to use gradle.startParameter to read system properties

I suggest reviewing only the last 2 commits as the first 2 only revert the previous changes.

Relevant diff: https://github.com/gradle/gradle-enterprise-build-validation-scripts/pull/682/files/64f48ec26c9fb43c51d62d281a9c3a99554342e1..632519d8c0cf14190fe3ac6608b72ac50fd791d4

@erichaagdev erichaagdev requested review from bigdaz, etiennestuder, jthurne and a team November 20, 2024 00:46
@erichaagdev erichaagdev self-assigned this Nov 20, 2024
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.

Since these changes are required for BVS, is there a good reason why the same problem won't manifest for other init-script consumers?

I'm OK with these changes to BVS, but I'd request changes to apply these to the reference init-script. Namely, I think we can avoid the duplicate definition of getInputParam(name) by changing the signature to getInputParam(gradle, name).

@erichaagdev
Copy link
Member Author

is there a good reason why the same problem won't manifest for other init-script consumers?

I expect this is a universal issue. In earlier versions of Gradle, what I discovered is that any modification to environment variables (changes, additions, etc.) are not reflected throughout the life of the Daemon process. It will be more common that you have Daemon reuse locally than in CI.

The other consideration is locally these environment variables are only included when invoking the scripts. In CI, the environment variables are always present in CI for every build invocation, though modifications are possible, it will be very infrequent.

I think this essentially makes it a non-issue for CI plugins. I believe this is the relevant Gradle issue: gradle/gradle#10483

I'd request changes to apply these to the reference init-script. Namely, I think we can avoid the duplicate definition of getInputParam(name) by changing the signature to getInputParam(gradle, name).

I can follow up with a PR. 👍

Copy link
Member

@etiennestuder etiennestuder left a comment

Choose a reason for hiding this comment

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

LGTM. @erichaagdev and I went through the PR review together.

@etiennestuder etiennestuder self-requested a review November 26, 2024 17:57
@erichaagdev erichaagdev merged commit 83e5519 into main Nov 26, 2024
4 checks passed
@erichaagdev erichaagdev deleted the erichaagdev/use-start-parameter branch November 26, 2024 19:34
erichaagdev added a commit to gradle/develocity-ci-injection that referenced this pull request Dec 3, 2024
Gradle 7.0.2 and earlier cannot reliably read system properties using
`System.getProperty` from init scripts. This PR adds reading system
properties using the `gradle.startParameter.systemPropertiesArgs` API.

Related BVS PR:
gradle/develocity-build-validation-scripts#682
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