-
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
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) #104454
Conversation
…2403)" (dotnet#104414) This reverts commit 5b86dca.
… the system installed zlib
Tagging subscribers to this area: @dotnet/area-system-io-compression |
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.
@pavelsavara @ilonatommy @kg @jkotas @jkoritzinsky @akoeplinger I need some help, I am still unable to get a successful build in wasm.
I can repro the exact same scenario used in the CI by executing these commands inside my Ubuntu WSL:
docker run -ti -d -v /home/carlos/repos/runtime:/home/carlos/repos/runtime -e ROOTFS_DIR=/crossrootfs/x64 -w /home/carlos/repos/runtime mcr.microsoft.com/dotnet-buildtools/prereqs:azurelinux-3.0-webassembly-amd64-net9.0
docker attach <GUID_HERE>
git config --global --add safe.directory /home/carlos/repos/runtime
./build.sh mono+libs+host+packs+libs.tests -c Release -arch wasm -os browser /p:MonoEnableAssertMessages=true /p:BrowserHost=linux /p:AotHostArchitecture=x64 /p:AotHostOS=linux
And the build is failing with the following error, which tells me that I am still unable to successfully link the system zlib (which I confirmed exists inside the crossrootfs folder) to mono/metadata:
/home/carlos/repos/runtime/src/mono/mono/metadata/debug-mono-ppdb.c:33:10: fatal error: 'zlib.h' file not found
#include <zlib.h>
^~~~~~~~
1 error generated.
emcc: error: '/home/carlos/repos/runtime/src/mono/browser/emsdk/bin/clang -target wasm32-unknown-emscripten -fignore-exceptions -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr -DEMSCRIPTEN -Werror=implicit-function-declaration --sysroot=/home/carlos/repos/runtime/src/mono/browser/emsdk/emscripten/cache/sysroot -Xclang -iwithsysroot/include/fakesdl -Xclang -iwithsysroot/include/compat -DCOMPILER_SUPPORTS_W_RESERVED_IDENTIFIER -DHAVE_CONFIG_H -DHAVE_SGEN_GC -DMONO_DLL_EXPORT -D_THREAD_SAFE -I/home/carlos/repos/runtime/artifacts/obj -I/home/carlos/repos/runtime/src/native -I/home/carlos/repos/runtime/artifacts/obj/mono/browser.wasm.Release/mono/metadata/../.. -I/home/carlos/repos/runtime/src/mono/mono/metadata/../.. -I/home/carlos/repos/runtime/src/mono/mono/metadata/.. -I/home/carlos/repos/runtime/src/native/public/. -I/home/carlos/repos/runtime/artifacts/obj/mono/browser.wasm.Release/mono/eglib -I/home/carlos/repos/runtime/src/mono/mono/eglib -fno-strict-aliasing -fwrapv -Wall -Wunused -Wmissing-declarations -Wpointer-arith -Wno-cast-qual -Wwrite-strings -Wno-switch -Wno-switch-enum -Wno-unused-value -Wno-attributes -Wno-format-zero-length -Wno-unused-function -Qunused-arguments -Wno-tautological-compare -Wno-parentheses-equality -Wno-self-assign -Wno-return-stack-address -Wno-constant-logical-operand -Wno-zero-length-array -Wno-asm-operand-widths -Wmissing-prototypes -Wstrict-prototypes -Wnested-externs -Werror=return-type -Werror=implicit-function-declaration -Werror=incompatible-pointer-types -O3 -DNDEBUG -std=gnu11 -g3 -fPIC -fvisibility=hidden -Wno-strict-prototypes -Wno-unused-but-set-variable -Wno-single-bit-bitfield-constant-conversion -Os -ffp-contract=off -MD -MT mono/metadata/CMakeFiles/metadata_objects.dir/debug-mono-ppdb.c.o -MF CMakeFiles/metadata_objects.dir/debug-mono-ppdb.c.o.d -c /home/carlos/repos/runtime/src/mono/mono/metadata/debug-mono-ppdb.c -o CMakeFiles/metadata_objects.dir/debug-mono-ppdb.c.o' failed (returned 1)
make[2]: *** [mono/metadata/CMakeFiles/metadata_objects.dir/build.make:257: mono/metadata/CMakeFiles/metadata_objects.dir/debug-mono-ppdb.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:415: mono/metadata/CMakeFiles/metadata_objects.dir/all] Error 2
make: *** [Makefile:136: all] Error 2
/home/carlos/repos/runtime/src/mono/mono.proj(765,5): error MSB3073: The command "cmake --build . --target install --config Release --parallel 12" exited with code 2.
Build FAILED.
@LoopedBard3 I think I need to remove |
Azure Pipelines successfully started running 1 pipeline(s). |
|
Let's collaborate on getting those fixed in the WASI PR. It will be easier to test and iterate there. It's important that this change makes it into Preview 7. |
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.
Approve this change to get this in and building. Please file a follow up issue on the platform manifest cleanup, and work with @pavelsavara on wasi fix.
@@ -279,6 +279,7 @@ | |||
<PlatformManifestFileEntry Include="libicuuc.a" IsNative="true" /> | |||
<!-- zlib-specific files --> | |||
<PlatformManifestFileEntry Include="libz.a" IsNative="true" /> | |||
<PlatformManifestFileEntry Include="zlibstatic.lib" IsNative="true" /> |
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.
is it intentional to ship zlibstatic.lib in our shared framework? That seems unusual as it's a dev-time resource - however I see other dev-time resources here like main.c
, driver.h
, etc. I take it you needed to add this to build - but I bet a better fix is to get all of this stuff out of the platform manifest since I don't think it needs to be fed to the SDK for conflict resolution @jkoritzinsky @dsplaisted.
I see we have a metadata flag ExcludeFromDataFiles
for that: https://github.com/dotnet/arcade/blob/76f733ee57811c38bb5b8e1ac9c6c50e92bc5dc9/src/Microsoft.DotNet.SharedFramework.Sdk/targets/sharedfx.targets#L249
I think we should review all the things added here, remove files that don't need to ship - if any, and apply ExcludeFromDataFiles
for the files that need to ship and don't actually need to go into the manifest. That can be done separately.
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.
I'll keep this in mind as a follow up.
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.
is it intentional to ship zlibstatic.lib in our shared framework?
zlibstatic.lib ships in the native aot runtime package only. It does not ship in the regular shared framework.
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.
When I implemented the shared framework SDK, no one really knew what needed to go into the manifest, so it requires/lists all files. If we know what types of files need to be in it, I can update the SDK to only validate (for the template case) or include (for the generative case) files that matter.
/ba-g All failures determined to be unrelated. |
@LoopedBard3 @caaavik-msft @DrewScoggins where can I see the runtime perf chart before and after this change? |
If you have specific tests in mind, we have an ADX dashboard and database. I have DMed these to you. Otherwise, if you want to just look at a few of the test results, we also have an allTestHistory that is updated once a day with the latest run data. Although it seems we currently have a pathing bug so you will need to remove the "reports/" from the url to view the actual test if the links give a 404. |
@carlossanlop, VMR jobs are failing with:
blocking SDK dotnet/sdk#42019. Do we need to set |
Yep, I've been notified and I'm looking into this.
No, that property needs to be set conditionally, plus it is already set to true in this failing case, so the problem is not there. |
Likely related improvements: |
We are seeing different results in the compressed payload in aspnetcore tests, 1 byte difference, is that expected with this switch? Wondering if a gzipped payload is supposed to be deterministic with the same compression level. |
There's no guarantee about compressed bytes being exactly the same from version to version. It's expected that updating the zlib implementation may change the exact compressed bytes generated. |
Contributes to: #101465
This PR reapplies the reverted changes due to a wasm break in the official build: #102403
It also includes an extra commit that fixes the NativeAOT build failure that only happened in the official build.