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

Execute Gradle in parent context in Windows scripts #29067

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mataha
Copy link
Contributor

@mataha mataha commented May 8, 2024

Context

Windows script gradlew.bat executes Gradle within its own, batch context. This causes every locally declared variable to be visible to underlying java process, potentially leaking information about used options and classpath.

To mitigate this, we jump to a non-existent label, forcing cmd.exe to stop processing the script, yet run the remaining commands within the current line in parent (most likely command-line when invoking gradlew directly) context. This introduces a harmless side-effect in form of corrupting the current title of the window cmd.exe was invoked in, thus it is reset to the default value.

This provides a few side benefits:

Issue: #10463

Contributor Checklist

  • Review Contribution Guidelines.
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team.
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective.
  • Provide unit tests (under <subproject>/src/test) to verify logic.
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes.
  • Ensure that tests pass sanity check: ./gradlew sanityCheck.
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest.

Reviewing cheatsheet

Before merging the PR, comments starting with

  • ❌ ❓must be fixed
  • 🤔 💅 should be fixed
  • 💭 may be fixed
  • 🎉 celebrate happy things

@mataha mataha requested review from a team as code owners May 8, 2024 19:16
@mataha mataha requested a review from jvandort May 8, 2024 19:16
@bot-gradle bot-gradle added from:contributor PR by an external contributor to-triage labels May 8, 2024
Copy link
Member

@ljacomet ljacomet left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

  • Can you explain the added deprecations?
  • See below for not needed change

Once these are addressed, someone with bat knowledge will need to take a look at the proposed changes.

Copy link
Member

Choose a reason for hiding this comment

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

❌ This file is generated and should not be modified in a PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ljacomet ljacomet added the pending:feedback Indicates that changes or additional info are required, and the issue will be closed without them label May 14, 2024
Windows script `gradlew.bat` executes Gradle within its own, batch context.
This causes every locally declared variable to be visible to underlying `java`
process, *potentially* leaking information about used options and classpath.

To mitigate this, we jump to a non-existent label, forcing `cmd.exe` to stop
processing the script, yet run the remaining commands within the current line
in parent (most likely command-line when invoking `gradlew` directly) context.
This introduces a harmless side-effect in form of corrupting the current title
of the window `cmd.exe` was invoked in, thus it is reset to the default value.

This provides a few side benefits:

  - no more 'Terminate batch job (Y/N)?' prompt
  - `call ` and `call` are used to directly set exit code, resolving gradle#4784
  - both sh and `cmd.exe` scripts become synchronized in terms of mechanism
    used to execute JVM as well as environment sanitization

Issue: gradle#10463

Signed-off-by: mataha <[email protected]>
@mataha
Copy link
Contributor Author

mataha commented May 17, 2024

  • Can you explain the added deprecations?

Since we're exiting the batch context with (goto) and resetting the exit code afterwards, there's no code to run after that line, which makes exitEnvironmentVar and its getters/setters obsolete.

I've considered removing it altogether - that said, it would break binary compatibility, so I've chosen to deprecate it instead to show that it's not used nor needed anymore; I'm not 100% set on that though. I just felt that exitEnvironmentVar removal should be communicated to the user somehow.

@github-actions github-actions bot removed the pending:feedback Indicates that changes or additional info are required, and the issue will be closed without them label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:contributor PR by an external contributor to-triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants