Skip to content
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

[NativeAOT-LLVM] Start running the wasi library tests #3027

Open
wants to merge 3 commits into
base: feature/NativeAOT-LLVM
Choose a base branch
from

Conversation

jsturtevant
Copy link

upstream stopped running the library tests for wasi with dotnet/runtime#112716. To ensure we still have coverage this adds them back on the branch here.

Note, I am not sure this will work as is. Still figuring out the how the build system works. Any pointers welcome :-)

fyi @pavelsavara

Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
@SingleAccretion
Copy link

SingleAccretion commented Mar 5, 2025

@jsturtevant the NAOT-LLVM libraries tests are defined here:

<!-- NativeAOT-LLVM: Just a few test suites. -->
<ItemGroup Condition="'$(TestNativeAot)' == 'true' and '$(TargetsWasm)' == 'true'">
<SmokeTestProject Remove="@(SmokeTestProject)" />
<SmokeTestProject Include="$(MSBuildThisFileDirectory)System.Collections.Specialized\tests\System.Collections.Specialized.Tests.csproj" />
<SmokeTestProject Include="$(MSBuildThisFileDirectory)System.Diagnostics.StackTrace\tests\System.Diagnostics.StackTrace.Tests.csproj" />
<!-- S.R.Tests aren't yet passing upstream on WASI. -->
<SmokeTestProject Include="$(MSBuildThisFileDirectory)System.Runtime\tests\System.Runtime.Tests\System.Runtime.Tests.csproj" Condition="'$(TargetsBrowser)' == 'true'" />
</ItemGroup>

It's intentionally only a very small subset of tests since they take a long time to compile.

- ${{ if eq(parameters.archType, 'wasm') }}:
- script: |
$(Build.SourcesDirectory)/build$(scriptExt) libs.tests -test -a ${{ parameters.archType }} -os ${{ parameters.osGroup }} -lc ${{ parameters.librariesConfiguration }} -rc $(buildConfigUpper) /p:TestNativeAot=true /p:RunSmokeTestsOnly=true
displayName: Build and run WebAssembly libraries tests

@pavelsavara
Copy link
Member

You can also have separate CI leg for Mono WASI and run tests there.

@SingleAccretion
Copy link

You can also have separate CI leg for Mono WASI and run tests there.

Well... I don't think we would have the resources (time/expertise) to keep it running.

@pavelsavara
Copy link
Member

@lewing said Azdo spend is fine.
We just disabled the same on the main CI, which was running it many times a day.
This is low-traffic branch in comparison. It will be cheaper here.

The interesting question is if you guys would be willing and able to keep up with the maintenance of it. I guess with James' help

@SingleAccretion
Copy link

SingleAccretion commented Mar 6, 2025

The interesting question is if you guys would be willing and able to keep up with the maintenance of it

That is my main concern. The logistics of having upstream CI downstream do not make sense to me. Let's consider what it implies:

  1. If we have some sort of break in this pipeline, it is likely (let's say 90%+) an upstream issue, since we don't have (many) diffs in the libraries portion.
  2. We ingest upstream infrequently and in bulk. It means every merge will have to deal with all these breaks at once. Merging is already full of mystery issues.
  3. Fixing upstream issues should be done upstream. It means that unless the downstream patches are accepted upstream verbatim, they will be a constant source of conflicts. This is on top of needing to coordinate changes across two repositories.

IMO, if upstream is not happy with running WASI CI on each PR, it should run it on some periodical pipeline that is monitored (we have many Jit pipelines like that - I also understand that it is not a "great" solution).

Alternatively, expanding the set of libraries tests that NAOT-LLVM runs would also be completely fine with me.

@dotnet/nativeaot-llvm

@yowl
Copy link
Contributor

yowl commented Mar 6, 2025

My 2 cents. Merging is not the most exciting task so I personally wouldn't relish having more breaks there. There seems, as an outsider, to be a bit of a disconnect with what Microsoft want. James (from MS) is working on the WIT side, which is great, and there has been some discussion to doing the mono work first (as that is the supported Wasm backend) which is fine and understandable, but it's not my personal/selfish interest. If someone else wants to keep the mono side working here, good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants