RFR (xs): JDK-8072611: (process) ProcessBuilder redirecting output to file should work with long file names (win)
Hi all, please review this small change at your convenience: http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.01/webrev/ It fixes a small issue which causes ProcessBuilder not to be able to open files to redirect into (when using Redirect.append) on windows, if that file name is longer than 255 chars. Kind Regards, Thomas Stuefe
Hi Thomas, Conventionally, File.deleteOnExit is used for file cleanup in the test. An unexpected exception could leave the file around. An alternative is to use try {...} finally {file.delete}; Fixed file names can sometime cause issues if they are left around by previous test runs or with a concurrent run of the same test on the same system. Can File.createTempFile be used with a suitably long prefix or suffix? It would ensure a unique file name. The rest looks good. Thanks, Roger On 2/9/15 8:30 AM, Thomas Stüfe wrote:
Hi all,
please review this small change at your convenience:
http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.01/webrev/
It fixes a small issue which causes ProcessBuilder not to be able to open files to redirect into (when using Redirect.append) on windows, if that file name is longer than 255 chars.
Kind Regards, Thomas Stuefe
Hi Thomas, the change looks good and I can sponsor it once it is reviewed. Just some small notes: - it seems that "getPath()" isn't used anywhere else in ProcessImpl_md.c and because it is static it can't be used anywhere else. So can you please remove it completely. - io_util_md.h has a list of files which use the prototypes like "pathToNTPath" before the declaration of the prototypes. Could you please also add ProcessImpl_md.c to this list - can you pleae place the new include in ProcessImpl_md.c as follows: #include "io_util.h" +#include "io_util_md.h" #include <windows.h> #include <io.h> I saw that system and local headers are already intermixed in that file but at I think at least we shouldn't introduce more disorder. - is "robocopy" really available on all supported Windows system. In http://en.wikipedia.org/wiki/Robocopy I read that it was introduced in Server 2008. I just verified that Java 8 only supports Server 2008 and above but just think of our internal test system:) Maybe we can use a program which is supported in every Windows version? Regards, Volker On Mon, Feb 9, 2015 at 2:30 PM, Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi all,
please review this small change at your convenience:
http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.01/webrev/
It fixes a small issue which causes ProcessBuilder not to be able to open files to redirect into (when using Redirect.append) on windows, if that file name is longer than 255 chars.
Kind Regards, Thomas Stuefe
Hi Roger, Volker, thanks for reviewing! I added all requested changes: @Roger - use now Files.createTempDirectory to get a unique directory - wrapped test in try/finally to cleanup afterwards @Volker - moved include up and added dependency to comment in io_util_md.h - Now I use hostname.exe, which I hope is part of every Windows :) http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.02/webrev/ Regards, Thomas On Mon, Feb 9, 2015 at 7:48 PM, Volker Simonis <volker.simonis@gmail.com> wrote:
Hi Thomas,
the change looks good and I can sponsor it once it is reviewed.
Just some small notes:
- it seems that "getPath()" isn't used anywhere else in ProcessImpl_md.c and because it is static it can't be used anywhere else. So can you please remove it completely.
- io_util_md.h has a list of files which use the prototypes like "pathToNTPath" before the declaration of the prototypes. Could you please also add ProcessImpl_md.c to this list
- can you pleae place the new include in ProcessImpl_md.c as follows:
#include "io_util.h" +#include "io_util_md.h" #include <windows.h> #include <io.h>
I saw that system and local headers are already intermixed in that file but at I think at least we shouldn't introduce more disorder.
- is "robocopy" really available on all supported Windows system. In http://en.wikipedia.org/wiki/Robocopy I read that it was introduced in Server 2008. I just verified that Java 8 only supports Server 2008 and above but just think of our internal test system:) Maybe we can use a program which is supported in every Windows version?
Regards, Volker
On Mon, Feb 9, 2015 at 2:30 PM, Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi all,
please review this small change at your convenience:
http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.01/webrev/
It fixes a small issue which causes ProcessBuilder not to be able to open files to redirect into (when using Redirect.append) on windows, if that file name is longer than 255 chars.
Kind Regards, Thomas Stuefe
Hi Thomas, I've just notices that your test has no copyright info and is indented with an indent width of 2 spaces instead of 4. If you don't mind I'll fix this before pushing so there's no need for a new webrev. Regards, Volker On Tue, Feb 10, 2015 at 10:58 AM, Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi Roger, Volker,
thanks for reviewing!
I added all requested changes:
@Roger - use now Files.createTempDirectory to get a unique directory - wrapped test in try/finally to cleanup afterwards @Volker - moved include up and added dependency to comment in io_util_md.h - Now I use hostname.exe, which I hope is part of every Windows :)
http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.02/webrev/
Regards, Thomas
On Mon, Feb 9, 2015 at 7:48 PM, Volker Simonis <volker.simonis@gmail.com> wrote:
Hi Thomas,
the change looks good and I can sponsor it once it is reviewed.
Just some small notes:
- it seems that "getPath()" isn't used anywhere else in ProcessImpl_md.c and because it is static it can't be used anywhere else. So can you please remove it completely.
- io_util_md.h has a list of files which use the prototypes like "pathToNTPath" before the declaration of the prototypes. Could you please also add ProcessImpl_md.c to this list
- can you pleae place the new include in ProcessImpl_md.c as follows:
#include "io_util.h" +#include "io_util_md.h" #include <windows.h> #include <io.h>
I saw that system and local headers are already intermixed in that file but at I think at least we shouldn't introduce more disorder.
- is "robocopy" really available on all supported Windows system. In http://en.wikipedia.org/wiki/Robocopy I read that it was introduced in Server 2008. I just verified that Java 8 only supports Server 2008 and above but just think of our internal test system:) Maybe we can use a program which is supported in every Windows version?
Regards, Volker
On Mon, Feb 9, 2015 at 2:30 PM, Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi all,
please review this small change at your convenience:
http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.01/webrev/
It fixes a small issue which causes ProcessBuilder not to be able to open files to redirect into (when using Redirect.append) on windows, if that file name is longer than 255 chars.
Kind Regards, Thomas Stuefe
Thank you Volker :) On Wed, Feb 11, 2015 at 2:36 PM, Volker Simonis <volker.simonis@gmail.com> wrote:
Hi Thomas,
I've just notices that your test has no copyright info and is indented with an indent width of 2 spaces instead of 4. If you don't mind I'll fix this before pushing so there's no need for a new webrev.
Regards, Volker
On Tue, Feb 10, 2015 at 10:58 AM, Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi Roger, Volker,
thanks for reviewing!
I added all requested changes:
@Roger - use now Files.createTempDirectory to get a unique directory - wrapped test in try/finally to cleanup afterwards @Volker - moved include up and added dependency to comment in io_util_md.h - Now I use hostname.exe, which I hope is part of every Windows :)
http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.02/webrev/
Regards, Thomas
On Mon, Feb 9, 2015 at 7:48 PM, Volker Simonis <volker.simonis@gmail.com
wrote:
Hi Thomas,
the change looks good and I can sponsor it once it is reviewed.
Just some small notes:
- it seems that "getPath()" isn't used anywhere else in ProcessImpl_md.c and because it is static it can't be used anywhere else. So can you please remove it completely.
- io_util_md.h has a list of files which use the prototypes like "pathToNTPath" before the declaration of the prototypes. Could you please also add ProcessImpl_md.c to this list
- can you pleae place the new include in ProcessImpl_md.c as follows:
#include "io_util.h" +#include "io_util_md.h" #include <windows.h> #include <io.h>
I saw that system and local headers are already intermixed in that file but at I think at least we shouldn't introduce more disorder.
- is "robocopy" really available on all supported Windows system. In http://en.wikipedia.org/wiki/Robocopy I read that it was introduced in Server 2008. I just verified that Java 8 only supports Server 2008 and above but just think of our internal test system:) Maybe we can use a program which is supported in every Windows version?
Regards, Volker
On Mon, Feb 9, 2015 at 2:30 PM, Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi all,
please review this small change at your convenience:
http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.01/webrev/
It fixes a small issue which causes ProcessBuilder not to be able to open files to redirect into (when using Redirect.append) on windows, if
that
file name is longer than 255 chars.
Kind Regards, Thomas Stuefe
participants (3)
-
Roger Riggs
-
Thomas Stüfe
-
Volker Simonis