[PATCH] Enable jtreg tests to run on WSL (Windows Subsystem for Linux)
Jonathan Gibbons
jonathan.gibbons at oracle.com
Mon Jan 7 23:32:43 UTC 2019
The core way to build jtreg is to run "make" directly. build-all.sh is
primarily intended as a convenient wrapper for those that want to use it.
Oracle still supports older releases of JDK for paying customers, which
is why we want to retain the option to build jtreg with JDK 7, as well
as with JDK 8.
I'll play with the ShellAction code to see if I can come up with
anything better.
-- Jon
On 01/07/2019 03:23 PM, Andrew Luo wrote:
>
> Hi Jon,
>
> Thanks for reviewing this. Yes, there is a bug there, it should be
> !result.isEmpty() at the end of joinWSLENV. Is it still the case that
> we support JDK 7? I got the impression from build-all.sh:48 that JDK
> 7 is no longer supported:
>
> ifcase${JAVA_VERSION} in 1.8*) false;; *) true; esac; then
>
> echo"Error: Expected a path to JDK with version 1.8, got version
> ${JAVA_VERSION}">&2
>
> exit1
>
> fi
>
> Or perhaps is this only the build-all.sh script that doesn’t support
> JDK 7, build jtreg itself does?
>
> As for the changes in ShellAction, the reason I did it that way
> instead of scanning through env, is because some environment variables
> have to be translated raw, others are paths, and others are path lists
> (multiple paths separated by ; in Windows). These all have to be
> added into WSLENV differently (in particular with the /p or /l
> suffix). Scanning through would be a bit error prone, as you’d have to
> decide if the variable is a raw string, path, or path list just using
> the value. Although it’s possible, it seems a bit error
> prone/clunky. If you have some preferred solution to this though, I’d
> be happy to make those changes.
>
> Thanks,
>
> -Andrew
>
> *From:*Jonathan Gibbons <jonathan.gibbons at oracle.com>
> *Sent:* Monday, January 7, 2019 2:20 PM
> *To:* Andrew Luo <andrewluotechnologies at outlook.com>;
> code-tools-dev at openjdk.java.net
> *Subject:* Re: [PATCH] Enable jtreg tests to run on WSL (Windows
> Subsystem for Linux)
>
> Andrew,
>
> There are some initial problems with your patch.
>
> 1. In ShellAction.joinWSLEnv ... it is still a self-imposed
> requirement that we can build jtreg with JDK 7 (to help support
> testing older releases of JDK.) In joinWSLEnv, you use String.join,
> which is @since 8, and so not available on 7. For now, I've written a
> replacement, but in doing do. I wonder if there is a bug in your
> original. In particular, at the end of the method, why do you add
> `suffix` if `result.isEmpty())`. I would have expected this to be a
> test if the result was _not_ empty.
>
> Anyway, here is my proposed replacement for the method:
>
> *private *String joinWSLENV(List<String> list, String suffix) {
>
> StringBuilder sb = *new *StringBuilder();
>
> String sep = *""*;
>
> *for *(String item : list) {
>
> *if *(item.trim().isEmpty() || item.toLowerCase().equals(*"path"*)) {
>
> *continue*;
>
> }
>
> sb.append(sep).append(item).append(suffix);
>
> sep = *":"*;
>
> }
>
> *return *sb.toString();
>
> }
>
> The code appends the ":" separator after the first item, and always
> appends the suffix. Is that what you believe it should do?
>
> 2. You added a new option, but did not add an entry into the resource
> file. This causes the jtreg build to fail one of its self-tests. I've
> created a placeholder entry, but it is at the semantic level I hate:
> "getFo(): gets a foo". If you can provide a more informative string,
> that would be better.
>
> $ hg diff src/share/classes/com/sun/javatest/regtest/tool/i18n.properties
> diff -r b6fabf6bf942
> src/share/classes/com/sun/javatest/regtest/tool/i18n.properties
> --- a/src/share/classes/com/sun/javatest/regtest/tool/i18n.properties
> Thu Dec 20 14:43:41 2018 -0800
> +++ b/src/share/classes/com/sun/javatest/regtest/tool/i18n.properties
> Mon Jan 07 13:38:40 2019 -0800
> @@ -257,6 +257,7 @@
> The name may contain '*' to match any sequence of characters. \
> For example, result.* or *.err.
> help.main.retain.arg=<pass,fail,error,all,file-pattern>,...
> +help.main.wslWindowsTarget.desc=(Windows only) Run with WSL targeting
> Windows
> help.main.dir.desc=Specify a base directory for test files and
> directories
> help.main.dir.arg=<dir>
> help.main.allowSetSecurityManager.desc=Allow agentVM tests to set a
> security manager. \
>
> 3. README. It's not OK to change the baseline JDK used to build jtreg.
> If there is a WSL-reason why JDK 8 is required to run jtreg in a WSL
> environment, that should be done as a runtime check.
>
> Beyond that, the changes to ShellAction feel a bit clunky, changing
> the non-WSL flow of the code more than might be thought necessary.
> After all, there's really only two changes here: inserting wsl.exe at
> the begtinning of the command, and computing/inserting WSLENV into the
> environment. Given that WSLENV only contains the names of the env
> variables, with suffixes as needed, I would have thought that a
> simpler impl would be to scan the keys in env to build WSLENV, using
> strings-in-switch to identify the suffixes, and do that all in a
> relatively simple helper method, without having to affect the mainline
> code to put values into the env.
>
> I've created CODETOOLS-7902357 to track this work.
> https://bugs.openjdk.java.net/browse/CODETOOLS-7902357
> <https://bugs.openjdk.java.net/browse/CODETOOLS-7902357>
> I've put a webrev containing your patch at
> http://cr.openjdk.java.net/~jjg/7902357/webrev.00/
> <http://cr.openjdk.java.net/%7Ejjg/7902357/webrev.00/>
> I've put a followup containing your patch + fixes for 1-2-3 above at
> http://cr.openjdk.java.net/~jjg/7902357/webrev.01/
> <http://cr.openjdk.java.net/%7Ejjg/7902357/webrev.01/>
>
> -- Jon
>
> On 01/07/2019 12:36 PM, Andrew Luo wrote:
>
> Hi Jon,
>
> For a general intro to WSL, see here:
>
> https://blogs.msdn.microsoft.com/wsl/2016/04/22/windows-subsystem-for-linux-overview/
>
> https://docs.microsoft.com/en-us/windows/wsl/faq
>
> These links might also be useful for reviewing these changes specifically:
>
> https://docs.microsoft.com/en-us/windows/wsl/interop
>
> https://blogs.msdn.microsoft.com/commandline/2017/12/22/share-environment-vars-between-wsl-and-windows/
>
> Thanks,
>
> -Andrew
>
> -----Original Message-----
>
> From: Jonathan Gibbons<jonathan.gibbons at oracle.com> <mailto:jonathan.gibbons at oracle.com>
>
> Sent: Monday, January 7, 2019 7:54 AM
>
> To: Andrew Luo<andrewluotechnologies at outlook.com>
> <mailto:andrewluotechnologies at outlook.com>;code-tools-dev at openjdk.java.net
> <mailto:code-tools-dev at openjdk.java.net>
>
> Subject: Re: [PATCH] Enable jtreg tests to run on WSL (Windows Subsystem for Linux)
>
> Andrew,
>
> I've been generally following the progress for supporting WSL, but now that I need to get more involved, (i.e. reviewing jtreg updates) can you point me at any reasonable general introductory material for WSL?
>
> -- Jon
>
> On 1/6/19 11:29 AM, Andrew Luo wrote:
>
> We recently added support for building OpenJDK on WSL (targeting
>
> Windows; targeting Linux always worked). However, running tests on
>
> WSL did not work, and we determined that some changes would be needed
>
> to jtreg to support this use case. In particular, jtreg needs to know
>
> if it is running on WSL targeting Windows or WSL targeting Linux (when
>
> targeting Linux, no changes are needed as it is the same as any other
>
> Linux system, but when targeting Windows jtreg needs to run shell
>
> scripts in a special manner - in particular, calling "wsl.exe sh"
>
> instead of just "sh" since it's a Windows boot JDK, as well as
>
> translating environment variables using WSLENV...)
>
> I've attached my patch with my proposed changes. I welcome any thoughts/feedback.
>
> (Seehttps://bugs.openjdk.java.net/browse/JDK-8215445 for more details
>
> about supporting WSL for building OpenJDK)
>
> Thanks,
>
> -Andrew
>
More information about the code-tools-dev
mailing list