Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
On Wed, 16 Nov 2022 15:12:41 GMT, Roger Riggs wrote:
>> Weibing Xiao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> the change according to review comment
>
> test/jdk/java/io/File/TempDirectoryNotExisting.java line 45:
>
>> 43:
>> 44: String userDir = System.getProperty("user.home");
>> 45: String timeStamp = System.currentTimeMillis() + "";
>
> A human readable string might be useful. For example,
> "2022-11-16T15:10:50.622334Z"
> `java.time.Instant.now().toString()`
root cause for JDK-8297528
Please revert the change Weibing and be sure to remove the test from
ProblemList when doing so (next week)
Please also ensure that you run tests against mach5 before integrating any
change in future,
-
PR: https://git.openjdk.org/jdk/pull/11174
Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
On Wed, 16 Nov 2022 15:06:14 GMT, Roger Riggs wrote:
>> Weibing Xiao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> the change according to review comment
>
> src/java.base/share/classes/jdk/internal/util/SystemProps.java line 52:
>
>> 50: * @return a boolean value
>> 51: */
>> 52: public static boolean checkIfWarningRequired() {
>
> I would rename the method to `checkIoTmpdir`; it will be easier to see the
> purpose in the call in System.
+1 I might suggest `isBadIoTmpdir` for method name
-
PR: https://git.openjdk.org/jdk/pull/11174
Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
On Wed, 16 Nov 2022 15:03:37 GMT, Roger Riggs wrote:
>> Weibing Xiao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> the change according to review comment
>
> src/java.base/share/classes/jdk/internal/util/SystemProps.java line 70:
>
>> 68: HashMap props = raw.cmdProperties();
>> 69:
>> 70: customTmpdir = props.get("java.io.tmpdir");
>
> Move the assignment to line 98:
>
> `customTmpdir = putIfAbsent(props, "java.io.tmpdir",
> raw.propDefault(Raw._java_io_tmpdir_NDX));`
>
> It will return null if the property is not already set and save a little bit.
> If it is set, it will return the custom directory.
I retract this suggestion, the `putIfAbsent` method does not return a value.
Moving the assignment to before line 113, would keep the references to
java.io.tmpdir together.
-
PR: https://git.openjdk.org/jdk/pull/11174
Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
On Wed, 16 Nov 2022 14:02:32 GMT, Weibing Xiao wrote:
>> print warning message for java.io.tmpdir when it is set through the command
>> line and the value is not good for creating file folder.
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> the change according to review comment
src/java.base/share/classes/java/lang/System.java line 2249:
> 2247: // Emit a warning if java.io.tmpdir is set via the command line
> to a directory that doesn't exist
> 2248: if (SystemProps.checkIfWarningRequired()) {
> 2249: System.err.println("WARNING: java.io.tmpdir location does
> not exist");
Change "location" to "directory", reflecting the exact check being done.
src/java.base/share/classes/jdk/internal/util/SystemProps.java line 48:
> 46:
> 47: /**
> 48: * check if warning for custom java.io.tmpdir is required
Capitialize and add a period at the end.
src/java.base/share/classes/jdk/internal/util/SystemProps.java line 52:
> 50: * @return a boolean value
> 51: */
> 52: public static boolean checkIfWarningRequired() {
I would rename the method to `checkIoTmpdir`; it will be easier to see the
purpose in the call in System.
src/java.base/share/classes/jdk/internal/util/SystemProps.java line 70:
> 68: HashMap props = raw.cmdProperties();
> 69:
> 70: customTmpdir = props.get("java.io.tmpdir");
Move the assignment to line 98:
`customTmpdir = putIfAbsent(props, "java.io.tmpdir",
raw.propDefault(Raw._java_io_tmpdir_NDX));`
It will return null if the property is not already set and save a little bit.
If it is set, it will return the custom directory.
test/jdk/java/io/File/TempDirectoryNotExisting.java line 45:
> 43:
> 44: String userDir = System.getProperty("user.home");
> 45: String timeStamp = System.currentTimeMillis() + "";
A human readable string might be useful. For example,
"2022-11-16T15:10:50.622334Z"
`java.time.Instant.now().toString()`
test/jdk/java/io/File/TempDirectoryNotExisting.java line 49:
> 47:
> 48: if (args.length != 0) {
> 49: if (args[0].equals("io")) {
This code to cover the cases could be a bit cleaner and without duplication as:
```
for (String arg : args) {
if (arg.equals("io")) {
...
} else if (arg.equals("nio")) {
...
} else {
throw Exception("unknown case: " + arg);
}
}
test/jdk/java/io/File/TempDirectoryNotExisting.java line 123:
> 121: .filter(line -> line.equalsIgnoreCase(ioWarningMsg))
> 122: .collect(Collectors.toList());
> 123: if (list.size() != 1) throw new Exception("counter of messages
> is not one, but " + list.size());
If there's an error, print the list of messages; it can be very informative to
see the unexpected messages, including the case where the exitValue is wrong.
-
PR: https://git.openjdk.org/jdk/pull/11174
Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
> print warning message for java.io.tmpdir when it is set through the command > line and the value is not good for creating file folder. Weibing Xiao has updated the pull request incrementally with one additional commit since the last revision: the change according to review comment - Changes: - all: https://git.openjdk.org/jdk/pull/11174/files - new: https://git.openjdk.org/jdk/pull/11174/files/0449a6d9..bcbba19e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11174=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11174=00-01 Stats: 13 lines in 2 files changed: 4 ins; 1 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/11174.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11174/head:pull/11174 PR: https://git.openjdk.org/jdk/pull/11174
Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
On Thu, 8 Sep 2022 19:22:37 GMT, Weibing Xiao wrote:
>> 8290313: Produce warning when user specified java.io.tmpdir directory
>> doesn't exist
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add the change for nio and update the code according to the comments
test/jdk/java/io/File/TempDirectoryNotExistingErrorMessage.java line 34:
> 32: import java.io.File;
> 33:
> 34: public class TempDirectoryNotExistingErrorMessage {
The tests look the same and maybe we can get away with one in java/io/File.
Renaming it something like The TempDirectoryDoesNotExist would make it a bit
more consistent with the tests in the Files test directory.
-
PR: https://git.openjdk.org/jdk/pull/9989
Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
On Thu, 8 Sep 2022 19:22:37 GMT, Weibing Xiao wrote:
>> 8290313: Produce warning when user specified java.io.tmpdir directory
>> doesn't exist
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add the change for nio and update the code according to the comments
Is the primary motive to catch scenarios where someone provides
-Djava.io.tmpdir= on the command line? I asked this via a comment in
the JBS issue last month and it would be useful to know if this is the scenario
that you are interested in. If it is, then I think we should look at a warning
at startup rather than delayed warning (or two warnings with the current patch).
src/java.base/share/classes/java/nio/file/TempFileHelper.java line 53:
> 51: // print out the error message for java.io.tmpdir setting
> 52: static {
> 53: if (!Files.isDirectory(tmpdir)) {
This won't work with a security manager set.
-
PR: https://git.openjdk.org/jdk/pull/9989
Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
> 8290313: Produce warning when user specified java.io.tmpdir directory doesn't > exist Weibing Xiao has updated the pull request incrementally with one additional commit since the last revision: add the change for nio and update the code according to the comments - Changes: - all: https://git.openjdk.org/jdk/pull/9989/files - new: https://git.openjdk.org/jdk/pull/9989/files/9f5a2983..cacf41d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9989=01 - incr: https://webrevs.openjdk.org/?repo=jdk=9989=00-01 Stats: 90 lines in 6 files changed: 69 ins; 12 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/9989.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9989/head:pull/9989 PR: https://git.openjdk.org/jdk/pull/9989
