-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WASM] Fix v8 trimming tests on windows #53698
[WASM] Fix v8 trimming tests on windows #53698
Conversation
Tagging subscribers to this area: @directhex Issue DetailsWork in progressCurrently the trimming tests are run from a shell script. This PR adds a cmd version when testing on windows.
|
Under https://github.com/dotnet/runtime/blob/main/eng/pipelines/runtime-linker-tests.yml#L73, add |
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsWork in progressCurrently the trimming tests are run from a shell script. This PR adds a cmd version when testing on windows.
|
@radical I just added it. However I see that the |
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? |
Maybe we can add |
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: |
Indeed, the |
It would be redundant in most other cases, I think, right? |
I just checked Also, should I try to push @radical 's suggestion and see what happens? |
The platform name is ok here, it is
Yes, let's try that so that you are not blocked on it. |
I think you can try to use |
Or maybe it would be possible to use |
Looks like |
So, it might be expanding it in |
Yes. Let's just duplicate the build jobs then for now, and set the different suffix in the second one. |
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 EDIT: actually I am passing it as
|
testGroup: innerloop | ||
timeoutInMinutes: 120 | ||
nameSuffix: Windows_Runtime_Release | ||
buildArgs: -s mono+libs -c $(_BuildConfig) -p:WasmBuildNative=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here .. /p:WasmBuildNative=false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can one of these be removed or are they both needed? Sorry for all the questions haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you. That makes a lot of sense
Looks like the current error is caused by some of the required tools for emscripten not being installed:
So if I understand correctly, since emscripten doesn't work without these dependencies, when we call @radical do you know what file is responsible for downloading the required tools for emsdk? |
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 Error: |
They are running on the build machine, IIUC. We would need to have v8 provisioned on that. |
How would this be done? I saw that cmake is added to the |
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. |
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. |
@lewing Is there still ongoing work w/ this PR? |
@radical this is now obsolete, right ? |
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 therun-v8.sh
one. So now, when the tests run, the cmd is file is executed.