-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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 <mataha@users.noreply.github.com>
Since we're exiting the batch context with 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 |
Thank you for your contribution! This PR has a valid DCO and meaningful change. The relevant team for this area will confirm the design of the implementation choices. |
It just occurred to me that this PR fixes #7883 (and its offshoots, like #12237) - after exec-like Proof-of-concept: @(goto) 2>nul & call & >>"%~dpnx0" (echo(@echo This line wasn't here before...) Maybe I should update the OP to reflect this... |
Context
Windows script
gradlew.bat
executes Gradle within its own, batch context. This causes every locally declared variable to be visible to underlyingjava
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 invokinggradlew
directly) context. This introduces a harmless side-effect in form of corrupting the current title of the windowcmd.exe
was invoked in, thus it is reset to the default value.This provides a few side benefits:
call
andcall
are used to directly set exit code, resolving Fix for Batch scripts not properly signalling a failure in a pipeline #4784cmd.exe
scripts become synchronized in terms of mechanism used to execute JVM as well as environment sanitizationIssue: #10463
Contributor Checklist
<subproject>/src/integTest
) to verify changes from a user perspective.<subproject>/src/test
) to verify logic../gradlew sanityCheck
../gradlew <changed-subproject>:quickTest
.Reviewing cheatsheet
Before merging the PR, comments starting with