-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Revert "Migrate to zlib-ng, part 2: consume it in runtime" #104414
Conversation
This reverts commit f5c9a5e.
Current error when building locally:
Trying to fix it we added
in |
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.
We have some issues building wasm/browser locally. We are not sure why CI passed yet.
The zlib-ng change is also making my attempt to upgrade to WASIp2 libc complicated.
I think we should merge the revert and try again without WASM targets, put them in similar bucket as other Mobile targets.
I wonder, was there reason to specifically enable it for wasm ?
/ba-g failures not connected |
@ilonatommy what build command did you run locally which reproed the problem? |
I'm surprised failures were encountered since the CI passed in my PR in all the runs: runtime, runtime-extra-platforms, runtime-nativeaot-outerloop, runtime-community. @ilonatommy @pavelsavara is there a wasm specific azp run I should also execute? |
@ilonatommy @kg reported
On their local systems. I don't understand what it means. @akoeplinger hinted that maybe it still tried to link the other zlib too. My issue with WASI + wasip2 is not merged yet into main, so you could not have see those. When I merged your zlib-ng changes I got
When I hacked that I got ton of other problems, for which I don't have log anymore. Sorry. We are willing to collaborate to figure it out. The other option is to merge your PR without wasm changes and get there in subsequent PR. We saw that the revert is for the official build reason and welcomed that we could get unblocked. |
|
Yes, I think this is the best option. I'm working on it. |
For me regular |
…2403)" (dotnet#104414) This reverts commit 5b86dca.
Regular runtime build for WASM: |
See https://dnceng.visualstudio.com/internal/_build/results?buildId=2487705 and the next few builds. |
@agocke I don't see any wasm failures. |
Oh, but tut there's a nativeaot failure related to my change:
I'm surprised that I did not see that one in the CI of my PR. I think I know how to fix that one. |
Is there an azp run that would execute the same or similar nativeaot command? In my PR I ran nativeaot-outerloop, and the failure did not show up there. |
There is no such command in dotnet/runtime. This is bug in the installed layout. We do not have any tests in dotnet/runtime that run validation against installed layout. e2e tests like that are in sdk and source build repos. |
Thanks. Fortunately I was able to repro it locally and also fix it locally with this commit: d232aaf |
The PR which switches to WASI preview 2 is #103752 the build problems of zlib-ng in WASI preview 2 are :
|
The failure in the codespaces is
|
…104454) * Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414) * Apply jkotas comment suggestion in configureplatform.cmake * Delete unnecessary comment in zlib-ng.cmake * Fix windows nativeaot failure happening when executing: build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
Reverts #102403
Looks like that change broke the official build.