Closed nurupo closed 3 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 73.11%. Comparing base (
ed2b60c
) to head (b3c3c49
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
The CI is kind of weird, it only partially tests the changes I have done to the Windows cross-compilation docker in this PR.
docker/docker-win32
and docker/docker-win64
build the docker image from this PR (i.e. they build sodium, opus and vpx), but nothing tests building toxcore using the resulting image.
ci/build-windows (32)
and ci/build-windows (64)
do build toxcore using a resulting image, but they use an older image from Dockerhub, instead of testing the one modified in the PR :\
Would be nice if docker/docker-win32
/docker/docker-win64
also built toxcore using the current image.
Anyway, I'm experiencing an issue locally where wine crashes when creating a 64-bit prefix, so the autotests for the 64-bit build of toxcore do not run. Wanted to see that failure happen on the CI, but alas CI uses a pre-PR image where everything works. This actually fails the entire build, not just autotests, as the wine prefix creation is done within bash's -e
option. Of course that's if you enable testing, which is not done by default. The wine crash seems to be some common bug that many are reporting, but there is no workaround for it, as far as I can see. Will try installing a newer version of wine from wine's own 3rd party Debian repository and see if that works. Here is the crash output if anyone is curious:
+ '[' true = true ']'
+ rm -rf /root/.wine
+ '[' x86_64 = i686 ']'
+ export WINEARCH=win64
+ WINEARCH=win64
+ winecfg
wine: created the configuration directory '/root/.wine'
002c:err:module:import_dll Loading library zlib1.dll (which is needed by L"C:\\windows\\system32\\user32.dll") failed (error c000007b).
0040:err:module:import_dll Loading library zlib1.dll (which is needed by L"C:\\windows\\system32\\user32.dll") failed (error c000007b).
002c:err:module:DelayLoadFailureHook failed to delay load user32.dll.CreateDialogParamW
wine: Call from 000000007B013D7E to unimplemented function user32.dll.CreateDialogParamW, aborting
wine: Unimplemented function user32.dll.CreateDialogParamW called at address 000000007B013D7E (thread 002c), starting debugger...
0040:err:module:import_dll Library user32.dll (which is needed by L"C:\\windows\\system32\\rundll32.exe") not found
0040:err:module:LdrInitializeThunk Importing dlls for L"C:\\windows\\system32\\rundll32.exe" failed, status c0000135
002c:err:seh:start_debugger Couldn't start debugger L"winedbg --auto 40 48" (2)
Read the Wine Developers Guide on how to set up winedbg or another debugger
wine: could not load kernel32.dll, status c0000135
-set_target_properties(toxcore_shared PROPERTIES OUTPUT_NAME toxcore)
+set_target_properties(toxcore_shared PROPERTIES OUTPUT_NAME toxcore WINDOWS_EXPORT_ALL_SYMBOLS ON)
@robinlinden that property seems to do nothing on MinGW on Linux. I did a tree
on the build dir, as well as the install prefix, and I don't see any .def
file.
Some post suggests that CMAKE_SUPPORT_WINDOWS_EXPORT_ALL_SYMBOLS
should be set for it to work on Linux MinGW,
I think I will be sticking with the gendef
command since it works.
The cmake build has the strict ABI option tuned on, so only the Tox symbols from the Tox public API headers (tox.h, toxav.h, toxencryptsave.h, tox_private.h, etc.) are listed in .def
, no internal symbols or those of dependencies.
Changed to use winehq-stable
package from WineHQ's repository, as that fixes the 64-bit Wine prefix creation crash.
Some autotests fail, but that's probably okay. For example, if you set -DUSE_IPV6=OFF
, then instead of 8 tests failing and timing out, you get only 2 failing. Furthermore, if you build toxcore with -DCMAKE_BUILD_TYPE=Debug
only 1 test fails (apparently that fixes the other test?). That remaining failing test passes on an older version of Wine (8.20 staging) and on a native Windows 10 system, so it seems to be a Wine bug.
Updated the CI to build and run the same Docker image, instead of building it but running an older version from Dockerhub. The CI still pushes the newly built image to Dockerhub. Had to split building and pushing into two steps as buildx doesn't allow two outputs at once https://github.com/docker/buildx/issues/316, so one step stores it locally with the docker daemon -- load: true
, and the following step re-uses the image from the local daemon and pushes it to Dockerhub -- push: true
(with the third step running the image stored with the local docker daemon).
Another thing that is probably worth explaining is that I removed the -O3
from toxcore's CFLAGS
as -DCMAKE_BUILD_TYPE="Release"
already sets that for us so it was redundant, and having -O3
with -DCMAKE_BUILD_TYPE="Debug"
(and disabled stripping) was messing the build up -- the resulting binaries ended up having only some debug symbols, gdb was able to tell the exact line number and filename of what the program is currently running, but couldn't show the 10-line code listing. Removing -O3
fixed that. That threw me off for a bit while debugging one of the test failures, thought my gdb was somehow too old to debug the binary or something, but no, I just used -O3
in a debug build.
I feel like calling the ci job just "windows" is misleading since we also have msvc. so "windows-mingw" and "windows-msvc" or similar would be more appropriate.
@Green-Sky looks good now?
Title contains "WIP"
code-review/reviewable Pending — 1 of 2 ✓ (iphydf, nurupo, robinlinden)
nurupo requested review from iphydf and robinlinden 2 minutes ago
That was done by Reviewable, I didn't manually request those. I guess Reviewable really wants @iphydf and @robinlinden to review this PR, even though @iphydf has approved it already.
Wanted to bump the toxcore dependencies for Windows cross-compilation before the release, but ended up changing a lot more things.
This change is