Make testing wasm on windows in runtimelab easier by using MSBuild IsOSPlatform not TargetOS

[yowl]

This PR changes the switch for .cmd/.sh to be based on $([MSBuild]::IsOSPlatform('windows')) instead of '$(TargetOS)' == 'windows' This helps in running the Wasm tests on Windows in the runtimelab branch, and should be ok here in runtime. Will help with future merging.

[ghost]

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This PR changes the switch for .cmd/.sh to be based on $([MSBuild]::IsOSPlatform('windows')) instead of '$(TargetOS)' == 'windows' This helps in running the Wasm tests on Windows in the runtimelab branch, and should be ok here in runtime. Will help with future merging.

Author:	yowl
Assignees:	-
Labels:	area-Infrastructure-libraries
Milestone:	-

[jkotas]

Thanks

[safern]

What is the motivation for this? If we want to run tests on wasm shouldn't the TargetOS be Browser?

[jkotas]

What is the motivation for this? If we want to run tests on wasm shouldn't the TargetOS be Browser?

That's exactly the problem. The tests are hosted on Windows, '$(TargetOS)' == 'windows' in the current code is false, and we end up trying to run .sh script on Windows that does not work well.

[safern]

I see. Shouldn't we instead do an and condition? '$(TargetOS)' == 'Browser' and ([MSBuild]::IsOSPlatform('windows'))? With this now, I wouldn't be able to build a test project on windows and then share it to run it on linux.

[jkotas]

build a test project on windows and then share it to run it on linux

Does this work today?

[safern]

It does if you have the runtime built on that target platform, but the tests should be platform agnostic. Not a big deal though, I don't think it is a common scenario, just trying to get the motivation behind doing this. A lot of our outputs depend on TargetOS so it jumped out that this will now depend on the host platform, so I wouldn't be able to produce a different output here.

[yowl]

I notice some other scripts use $(TestWrapperTargetsWindows)' == 'true' e.g.

runtime/src/tests/Common/CLRTest.Execute.targets

Line 61 in bbc6e7f

<ExecutionScriptKind Include="Batch" Condition="'$(TestWrapperTargetsWindows)' == 'true'" />

Is that relevant to this discussion?

[ViktorHofer]

See,

runtime/src/libraries/externals.csproj

Line 58 in 24a602b

<Exec Command="chmod +x $(TestHostRootPath)%(DotnetExe.Filename)%(DotnetExe.Extension)" Condition="'$(TargetOS)' != 'windows' and '$(OS)' != 'Windows_NT'"/>

which is similar but works as expected. I believe the right change would be to do the same here as Santi already said.

[yowl]

I have a similar problem with

runtime/src/tests/Directory.Build.props

Lines 140 to 143 in bbc6e7f

	<PropertyGroup>
	<TestWrapperTargetsWindows>false</TestWrapperTargetsWindows>
	<TestWrapperTargetsWindows Condition=" ('$(TargetsWindows)' != '' And '$(TargetsWindows)' ) OR ('$(TargetOS)' == 'Android' And '$(TargetArchitecture)' == 'arm64' )">true</TestWrapperTargetsWindows>
	</PropertyGroup>

For testing the Wasm tests on windows the properties are set:

So TestWrapperTargetsWindows is set false and the tests fail as the runner tries to run the .sh versions. Is the suggestion at #46105 (comment) appropriate here also?

[yowl]

Thanks for the feedback, I've changed it as requested, and changed https://github.com/dotnet/runtime/pull/46105/files#diff-95dbd50260986d2d3ad1a2a725d2bb0b240e8cbfd00e98f38b63bebcc2b6a64d being the other place I had an issue.

[ViktorHofer]

I'm not a big fan of this change. Usually you only need an IsOSPlatform or $(OS) equals check if you invoke something that isn't supported on a certain OS. In my previous example, the OS condition is added as chmod is invoked which doesn't exist on Windows.

In the eng/testing/tests.targets cases, I don't see anything being invoked which would hinder cross-compiling hence I believe the existing conditions are fine as-is. Thoughts?

[jkotas]

#45545 is fixing the browser/wasm build on Windows. The next step would be to also allow running the browser/wasm tests on Windows. It is where this PR is going. cc @steveisok

@ViktorHofer What should be the command one uses to build and run the browser/wasm tests on Windows? Should it be something as simple as dotnet build /t:Test /p:TargetOS=browser /p:TargetArchitecture=wasm (assuming you have the dependencies setup)? Once we have the experience figured out, it should be easier to work through the change required in the msbuild files.

[yowl]

"if you invoke something that isn't supported on a certain OS". RunnerTemplate.sh is not supported on Windows, so as the $(TargetOS)' != 'windows' but the tests are running on Windows, as is the case with running WebAssembly tests in node.js (or any other wasm runtime such as wasmer I suppose) on Windows, you have this problem. I can continue managing the problem in runtimelab only if you don't like it.

Edit: my comment above superceeded by #46105 (comment)

[ViktorHofer]

Thanks for providing additional context on this change @jkotas.

@ViktorHofer What should be the command one uses to build and run the browser/wasm tests on Windows? Should it be something as simple as dotnet build /t:Test /p:TargetOS=browser /p:TargetArchitecture=wasm (assuming you have the dependencies setup)?

Before wasm, TargetOS always had an unambiguous meaning and it inferred the OS it's running on. If that isn't true anymore, do we need to rely on an extra property like RuntimeOS? Taking the current machine's OS isn't ideal as you wouldn't be able to cross-build the wasm tests.

RunnerTemplate.sh is not supported on Windows, so as the $(TargetOS)' != 'windows' but the tests are running on Windows

This is true for test invocation but the code path that you are touching is mainly about building the tests and preparing them for being run in CI. With your changes, we can't cross-build Unix tests anymore on Windows and vice-versa.

[jkotas]

The cross-compilation is always more advanced scenario. It does not always work, it requires special environment setup, and it is not unusual for it to need extra arguments.

How about this:

• Command to build and run browser/wasm tests on Windows: dotnet build /t:Test /p:TargetOS=browser /p:TargetArchitecture=wasm
• Command to build the browser/wasm tests on Windows with the test wrappers cross-compiled for Unix: dotnet build /p:TargetOS=browser /p:TargetArchitecture=wasm /p:TestWrapperTargetsWindows=false

[ViktorHofer]

I would like @safern to share his opinion regarding cross building and additional arguments as well. It's not yet clear to me, what the ideal solution would look like.

[ericstj]

@safern can you have a look?

[safern]

How about this:

I think that proposal makes sense to me. (#46105 (comment))

Anyway, with that solution the current change is broken, because we have:

<RunScriptInputName Condition="'$(TargetOS)' == 'Browser'">WasmRunnerTemplate.sh</RunScriptInputName>

Which is at the end of the RunScriptInputName declaration, so that means that since TargetOS==Browser it's value is going to be WasmRunnerTemplate.sh.

Are there any scenarios interesting to cross-compile other than Browser?

[safern]

@yowl @jkotas is this still needed? It seems like there is an incomplete discussion and this has conflicts already.

[yowl]

@safern I notice mono added support for building on WIndows recently, I don't if the testing is enabled on Windows also? But maybe those changes have made this obsolete. I did try merging those mono changes into runtimelab (NativeAOT-LLVM branch), but haven't finished that.

[safern]

@safern I notice mono added support for building on WIndows recently, I don't if the testing is enabled on Windows also?

I'm not sure... @steveisok do you know?

@yowl so it seems like we can close this PR on the meantime and re-visit if needed?

[yowl]

@safern fine with me.

[steveisok]

@safern I notice mono added support for building on WIndows recently, I don't if the testing is enabled on Windows also?

I'm not sure... @steveisok do you know?

@yowl so it seems like we can close this PR on the meantime and re-visit if needed?

@radekdoulik has been the one brining up windows and I do believe he has tests working. Unsure if it made its way up yet.