Skip to content

Add jdk.io.File.enableADS to list of immutable system properties #20721

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 25, 2022
Merged

Add jdk.io.File.enableADS to list of immutable system properties #20721

merged 3 commits into from
Jul 25, 2022

Conversation

robbavey
Copy link
Contributor

This commit adds jdk.io.File.enableADS to the list of immutable system properties, to enable the property
to be passed on the command line, rather than set at runtime.

Relates: elastic/logstash#14075
Signed-off-by: Rob Bavey [email protected]

Fixes #?

Context

A recent update to the JDK, present in 11.0.15+10, 17.0.3+7 introduced functionality
to check the validity of Windows file paths, rejecting, for example, paths with a :
present anywhere other than immediately following the drive letter. Unfortunately, this has the
side effect of throwing exceptions when attempting to write to the NUL: device.

This validity check is gated behind a property jdk.io.File.enableADS, which is set to false for
11.0.15+10, 17.0.3+7 releases of the JDK (it will be set to true for the next set of JDK releases)
This is causing our gradle jobs to fail, as the property is not set when the gradle daemon starts, as the
property is set after the test to determine the value is performed. This will fail for any Windows builds that
use the affected JDKs, and attempt to write to the NUL: device

Relates: elastic/logstash#14075

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

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

@robbavey
Add jdk.io.File.enableADS to list of immutable system properties
80e5a95 
This commit adds `jdk.io.File.enableADS` to the list of immutable system properties, to enable the property
to be passed on the command line, rather than set at runtime.

Relates: elastic/logstash#14075
Signed-off-by: Rob Bavey <[email protected]>
@bot-gradle bot-gradle added the from:contributor PR by an external contributor label May 10, 2022
@donat donat requested a review from h0tk3y June 22, 2022 15:00
h0tk3y
h0tk3y approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@h0tk3y h0tk3y left a comment

Choose a reason for hiding this comment

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

I verified that the behavior that depends on the property jdk.io.File.enableADS, namely handling of the paths to special Windows devices like NUL:, cannot be switched by modifying the property in a running JVM. So it is indeed another "immutable" system property, and adding it to the list seems to be reasonable.

@blindpirate
Copy link
Collaborator

@h0tk3y this branch seems to be a bit old, please either rebase/merge to latest master, or trigger a pre-tested commit directly (which merges the HEAD of this branch to master automatically).

@h0tk3y h0tk3y added this to the 7.6 RC1 milestone Jul 22, 2022
@h0tk3y
Copy link
Member

h0tk3y commented Jul 22, 2022

@bot-gradle test ReadyForNightly

@gradle gradle deleted a comment from h0tk3y Jul 22, 2022
@bot-gradle
Copy link
Collaborator

OK, I've already triggered the following builds for you:

@robbavey robbavey requested a review from hythloda as a code owner July 25, 2022 08:15
@h0tk3y
Copy link
Member

h0tk3y commented Jul 25, 2022

@bot-gradle test and merge

@gradle gradle deleted a comment from blindpirate Jul 25, 2022
@bot-gradle
Copy link
Collaborator

Your PR is queued. See the queue page for details.

@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@bot-gradle bot-gradle merged commit 0f1793e into gradle:master Jul 25, 2022
@big-guy
Copy link
Member

big-guy commented Jul 25, 2022

@h0tk3y could you also add this to the list of immutable properties in gradle_daemon.adoc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@h0tk3y h0tk3y h0tk3y approved these changes

@hythloda hythloda Awaiting requested review from hythloda

Assignees

@h0tk3y h0tk3y

Labels

from:contributor PR by an external contributor

Projects

None yet

Milestone

7.6 RC1

Development

Successfully merging this pull request may close these issues.

5 participants

@robbavey @blindpirate @h0tk3y @bot-gradle @big-guy