[WASM] Fix v8 trimming tests on windows #53698

Daniel-Genkin · 2021-06-03T20:23:06Z

Currently the trimming tests are run from a shell script. This PR adds a cmd version when testing on windows.

Why is this needed:
Previously when running the tests on windows, when the run-v8.sh file was executed, the file would open in the default .sh file viewer. The tests were not run. Thus they would fail by default.

What changed:
Now when building the tests on windows a run-v8.cmd file is generated instead of the run-v8.sh one. So now, when the tests run, the cmd is file is executed.

ghost · 2021-06-03T20:23:11Z

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

Issue Details

Work in progress

Currently the trimming tests are run from a shell script. This PR adds a cmd version when testing on windows.

Author:	Daniel-Genkin
Assignees:	-
Labels:	`area-Infrastructure-mono`
Milestone:	-

radical · 2021-06-04T15:07:26Z

Under https://github.com/dotnet/runtime/blob/main/eng/pipelines/runtime-linker-tests.yml#L73, add - Browser_wasm_win, so these tests can be run with windows. That would validate the PR too.

ghost · 2021-06-04T16:37:05Z

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Work in progress

Currently the trimming tests are run from a shell script. This PR adds a cmd version when testing on windows.

Author:	Daniel-Genkin
Assignees:	-
Labels:	`arch-wasm`, `area-Infrastructure-mono`
Milestone:	-

Daniel-Genkin · 2021-06-04T17:25:22Z

Under https://github.com/dotnet/runtime/blob/main/eng/pipelines/runtime-linker-tests.yml#L73, add - Browser_wasm_win, so these tests can be run with windows. That would validate the PR too.

@radical I just added it. However I see that the Build Analysis check is now failing due to this change with the message job name build_Browser_wasm_release_Runtime_Release appears more than once. Job names must be unique within a pipeline. Can you please explain what this means and if there is something else I should do to address this? Looks like the other job in this file lists multiple platforms, why doesn't it have this issue?

radekdoulik · 2021-06-04T17:42:02Z

I think that's because we don't add platform to job name. @akoeplinger do you know what would be best way to handle that?

I think the easiest would be duplicate the linker tests jobs and use different name suffix there. Maybe there's more elegant solution?

Would it make sense to add platform to job name? Would that cause troubles in other places?

radical · 2021-06-04T17:43:35Z

Maybe we can add parameters.hostedOS to the name?

radical · 2021-06-04T17:47:22Z

Would this work?

diff --git a/eng/pipelines/runtime-linker-tests.yml b/eng/pipelines/runtime-linker-tests.yml
index 836b54c849a..e2d7526e5b6 100644
--- a/eng/pipelines/runtime-linker-tests.yml
+++ b/eng/pipelines/runtime-linker-tests.yml
@@ -71,10 +71,11 @@ jobs:
     buildConfig: release
     platforms:
     - Browser_wasm
+    - Browser_wasm_win
     jobParameters:
       testGroup: innerloop
       timeoutInMinutes: 120
-      nameSuffix: Runtime_Release
+      nameSuffix: ${{ format('{0}_Runtime_Release', parameters.hostedOs) }}
       buildArgs: -s mono+libs -c $(_BuildConfig) -p:WasmBuildNative=false
       extraStepsTemplate: /eng/pipelines/libraries/execute-trimming-tests-steps.yml
       extraStepsParameters:

radekdoulik · 2021-06-04T17:50:28Z

Indeed, the hostedOS is shorter. It would be nice to add it in eng/pipelines/common/global-build-job.yml to the job name, I am not sure about the consequences though.

radical · 2021-06-04T17:52:50Z

Indeed, the hostedOS would be shorter. It would be nice to add it in eng/pipelines/common/global-build-job.yml to the job name, I am not sure about the consequences though.

It would be redundant in most other cases, I think, right?
And this naming change might break scripts, or queries for people anyway. But @akoeplinger would be a better judge of that. Meanwhile, we can try it in this PR, and continue to test the changes being made here.

Daniel-Genkin · 2021-06-04T17:54:54Z

I just checked eng/pipelines/common/global-build-job.yml and on line 126 I see this
- ${{ if eq(parameters.nameSuffix, 'Browser_wasm_Windows') }}:.
Is there a chance that the new platform should be Browser_wasm_Windows instead of Browser_wasm_win?

Also, should I try to push @radical 's suggestion and see what happens?

radekdoulik · 2021-06-04T18:01:04Z

I just checked eng/pipelines/common/global-build-job.yml and on line 126 I see this
- ${{ if eq(parameters.nameSuffix, 'Browser_wasm_Windows') }}:.
Is there a chance that the new platform should be Browser_wasm_Windows instead of Browser_wasm_win?

The platform name is ok here, it is Browser_wasm_win. The name suffix is set in eng/pipelines/runtime-staging.yml for that build. I am not sure whether we would need to update the machine certs here as well. If so, then we can check the platform instead of name suffix.

Also, should I try to push @radical 's suggestion and see what happens?

Yes, let's try that so that you are not blocked on it.

radekdoulik · 2021-06-04T18:11:24Z

/eng/pipelines/runtime-linker-tests.yml (Line: 78, Col: 19): Key not found 'hostedOs'

I think you can try to use parameters.platform here instead of hostedOs. The hostedOs is not a parameter of the global build job.

radekdoulik · 2021-06-04T18:14:14Z

Or maybe it would be possible to use parameters.platform.hostedOs.

Daniel-Genkin · 2021-06-04T18:15:52Z

/eng/pipelines/runtime-linker-tests.yml (Line: 78, Col: 19): Key not found 'platform'

Looks like platform isn't available either? So then parameters.platform.hostedOs wouldn't work either right?

radical · 2021-06-04T18:18:36Z

/eng/pipelines/runtime-linker-tests.yml (Line: 78, Col: 19): Key not found 'platform'

Looks like platform isn't available either? So then parameters.platform.hostedOs wouldn't work either right?

So, it might be expanding it in runtime-linker-tests.yml itself, instead of later when it has the parameters!

radekdoulik · 2021-06-04T18:19:25Z

Yes. Let's just duplicate the build jobs then for now, and set the different suffix in the second one.

Daniel-Genkin · 2021-06-04T18:26:17Z

Ok looks like that worked. I see it in the list:

Daniel-Genkin · 2021-06-04T18:47:55Z

Looks like this will not be as simple as just adding a new task that runs a .cmd instead of a .sh. The tests now failed because it was trying to pass parameters to a script with the wrong syntax. If I'm not mistaken passing new properties should be done via /p:[PROPERTY] whereas currently it is -p:[PROPERTY]. So looks like Powershell is misinterpreting the command:
Log error: https://dev.azure.com/dnceng/public/_build/results?buildId=1171882&view=logs&j=292a4b20-7ee9-5b07-a8c9-fb02f01d70e6&t=5334d3ce-6cfc-51b3-2ed7-cd53a427c8f7&l=17

EDIT: actually I am passing it as /p: (see below). Is there some other place I should do this as well?

  extraStepsParameters:
          extraTestArgs: '/p:WasmBuildNative=false'

radical · 2021-06-04T18:50:35Z

eng/pipelines/runtime-linker-tests.yml

+      testGroup: innerloop
+      timeoutInMinutes: 120
+      nameSuffix: Windows_Runtime_Release
+      buildArgs: -s mono+libs -c $(_BuildConfig) -p:WasmBuildNative=false


here .. /p:WasmBuildNative=false

Can one of these be removed or are they both needed? Sorry for all the questions haha.

Good question! The first one is for the build itself. And the second one is for the extraStep, which runs the tests.

extraTestArgs is being used in only one place, to build the the command line for running tests. So, the first one wouldn't imply the second.

Sorry for all the questions haha.

Feel free to keep asking ;)

Ok thank you. That makes a lot of sense

Daniel-Genkin · 2021-06-07T20:39:57Z

Looks like the current error is caused by some of the required tools for emscripten not being installed:

Warning: The SDK/tool 'node-14.15.5-64bit' cannot be activated since it is not installed! Skipping this tool...
Warning: The SDK/tool 'python-3.9.2-1-64bit' cannot be activated since it is not installed! Skipping this tool...
Warning: The SDK/tool 'java-8.152-64bit' cannot be activated since it is not installed! Skipping this tool...
Warning: The SDK/tool 'releases-upstream-72f4ec97fbc7ec16c15ae68a75b0a257b2835160-64bit' cannot be activated since it is not installed! Skipping this tool...
Warning: The SDK/tool 'sdk-releases-upstream-72f4ec97fbc7ec16c15ae68a75b0a257b2835160-64bit' cannot be activated since it is not installed! Skipping this tool...

So if I understand correctly, since emscripten doesn't work without these dependencies, when we call emsdk activate it does nothing. So the path is incorrect and thus the build fails.

@radical do you know what file is responsible for downloading the required tools for emsdk?

…om/Daniel-Genkin/runtime into wasm-run-v8-trimming-tests-windows

Daniel-Genkin · 2021-06-08T14:23:47Z

Looks like the build passes now! Now just the tests are failing since v8 is not installed. @radical @radekdoulik I searched through the repo and found that v8 is installed for Helix on Linux but didn't find anything for the trimming tests or for Windows. Are these tests running on Helix? Looks like there is no send to Helix step, so I would assume no right? If not, should I install jsvu and v8 similarly to how cmake it installed?

Error: 'v8' is not recognized as an internal or external command

radical · 2021-06-08T19:12:26Z

They are running on the build machine, IIUC. We would need to have v8 provisioned on that.
cc @radekdoulik @akoeplinger

Daniel-Genkin · 2021-06-08T19:34:24Z

We would need to have v8 provisioned on that.

How would this be done? I saw that cmake is added to the global.json and there is a script to download and install it. Is this what you mean I would need to do for jsvu and v8?

radical · 2021-06-08T20:44:28Z

We would need to have v8 provisioned on that.

How would this be done? I saw that cmake is added to the global.json and there is a script to download and install it. Is this what you mean I would need to do for jsvu and v8?

I'm not sure how it is being done on windows. Radek had provisioned them in the vm image being used for helix. I'm not sure if the same is used for the build too.

This is where he enabled the tests on helix. I don't see any extra PATHs being set.

@akoeplinger or @radekdoulik would be able to provide better answers.

radekdoulik · 2021-06-10T10:34:03Z

The build doesn't have V8 provisioned yet. I am working on a new docker image to use for build, in these PRs dotnet/dotnet-buildtools-prereqs-docker#461, #53602.

I did this mostly to have emscripten provisioned. I left jsvu and V8/spidermonkey in place too, so once this is finished, the V8 will be available and it will be possible to run these tests.

steveisok · 2021-08-31T13:58:50Z

@lewing Is there still ongoing work w/ this PR?

pavelsavara · 2021-12-06T10:17:25Z

@radical this is now obsolete, right ?

Daniel-Genkin added 4 commits June 3, 2021 13:19

Added windows support for running trimming tests

0e4aae8

Fixed up the issues

ebcc279

work in progress

5aba37c

fixed a typo

60b3411

dotnet-issue-labeler bot added the area-Infrastructure-mono label Jun 3, 2021

Daniel-Genkin marked this pull request as ready for review June 4, 2021 14:49

Daniel-Genkin requested a review from marek-safar as a code owner June 4, 2021 14:49

thaystg requested a review from radical June 4, 2021 15:03

radical added the arch-wasm WebAssembly architecture label Jun 4, 2021

radical requested review from lewing and radekdoulik June 4, 2021 16:37

Enabled testing browser_wasm on windows

6d90eba

Attempt to fix the Build Analysis check failure

3f014b8

Attempt to fix build analysis check failure

3614887

Attempt to fix build analysis check failure (3)

31e664b

radical reviewed Jun 4, 2021

View reviewed changes

fixed syntax error

5425b36

Daniel-Genkin added 3 commits June 7, 2021 17:12

Merge branch 'dotnet:main' into wasm-run-v8-trimming-tests-windows

f7d6e3d

addressed SSL issue (Thanks Radek)

89361d5

Merge branch 'wasm-run-v8-trimming-tests-windows' of https://github.c…

ade3108

…om/Daniel-Genkin/runtime into wasm-run-v8-trimming-tests-windows

This was referenced Jun 9, 2021

Pipeline dependency issue: Artifact CoreCLRProduct bits missing. #53948

Closed

Pipeline dependency issue: Artifact CoreCLRProduct bits missing. #53842

Closed

Daniel-Genkin changed the title ~~[wasm] Fix v8 trimming tests on windows~~ [WASM] Fix v8 trimming tests on windows Jun 16, 2021

terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021

pavelsavara removed the community-contribution Indicates that the PR has been added by a community member label Jul 26, 2021

marek-safar assigned pavelsavara Nov 22, 2021

pavelsavara closed this Dec 6, 2021

ghost locked as resolved and limited conversation to collaborators Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WASM] Fix v8 trimming tests on windows #53698

[WASM] Fix v8 trimming tests on windows #53698

Daniel-Genkin commented Jun 3, 2021 •

edited

Loading

ghost commented Jun 3, 2021

Work in progress

radical commented Jun 4, 2021

ghost commented Jun 4, 2021

Work in progress

Daniel-Genkin commented Jun 4, 2021 •

edited

Loading

radekdoulik commented Jun 4, 2021

radical commented Jun 4, 2021

radical commented Jun 4, 2021

radekdoulik commented Jun 4, 2021 •

edited

Loading

radical commented Jun 4, 2021

Daniel-Genkin commented Jun 4, 2021 •

edited

Loading

radekdoulik commented Jun 4, 2021

radekdoulik commented Jun 4, 2021

radekdoulik commented Jun 4, 2021

Daniel-Genkin commented Jun 4, 2021 •

edited

Loading

radical commented Jun 4, 2021

radekdoulik commented Jun 4, 2021

Daniel-Genkin commented Jun 4, 2021

Daniel-Genkin commented Jun 4, 2021 •

edited

Loading

radical Jun 4, 2021

Daniel-Genkin Jun 4, 2021 •

edited

Loading

radical Jun 4, 2021

Daniel-Genkin Jun 4, 2021

Daniel-Genkin commented Jun 7, 2021 •

edited

Loading

Daniel-Genkin commented Jun 8, 2021 •

edited

Loading

radical commented Jun 8, 2021

Daniel-Genkin commented Jun 8, 2021

radical commented Jun 8, 2021

radekdoulik commented Jun 10, 2021

steveisok commented Aug 31, 2021

pavelsavara commented Dec 6, 2021

[WASM] Fix v8 trimming tests on windows #53698

[WASM] Fix v8 trimming tests on windows #53698

Conversation

Daniel-Genkin commented Jun 3, 2021 • edited Loading

ghost commented Jun 3, 2021

Work in progress

radical commented Jun 4, 2021

ghost commented Jun 4, 2021

Work in progress

Daniel-Genkin commented Jun 4, 2021 • edited Loading

radekdoulik commented Jun 4, 2021

radical commented Jun 4, 2021

radical commented Jun 4, 2021

radekdoulik commented Jun 4, 2021 • edited Loading

radical commented Jun 4, 2021

Daniel-Genkin commented Jun 4, 2021 • edited Loading

radekdoulik commented Jun 4, 2021

radekdoulik commented Jun 4, 2021

radekdoulik commented Jun 4, 2021

Daniel-Genkin commented Jun 4, 2021 • edited Loading

radical commented Jun 4, 2021

radekdoulik commented Jun 4, 2021

Daniel-Genkin commented Jun 4, 2021

Daniel-Genkin commented Jun 4, 2021 • edited Loading

radical Jun 4, 2021

Choose a reason for hiding this comment

Daniel-Genkin Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

radical Jun 4, 2021

Choose a reason for hiding this comment

Daniel-Genkin Jun 4, 2021

Choose a reason for hiding this comment

Daniel-Genkin commented Jun 7, 2021 • edited Loading

Daniel-Genkin commented Jun 8, 2021 • edited Loading

radical commented Jun 8, 2021

Daniel-Genkin commented Jun 8, 2021

radical commented Jun 8, 2021

radekdoulik commented Jun 10, 2021

steveisok commented Aug 31, 2021

pavelsavara commented Dec 6, 2021

Daniel-Genkin commented Jun 3, 2021 •

edited

Loading

Daniel-Genkin commented Jun 4, 2021 •

edited

Loading

radekdoulik commented Jun 4, 2021 •

edited

Loading

Daniel-Genkin commented Jun 4, 2021 •

edited

Loading

Daniel-Genkin commented Jun 4, 2021 •

edited

Loading

Daniel-Genkin commented Jun 4, 2021 •

edited

Loading

Daniel-Genkin Jun 4, 2021 •

edited

Loading

Daniel-Genkin commented Jun 7, 2021 •

edited

Loading

Daniel-Genkin commented Jun 8, 2021 •

edited

Loading