feat: Update and improve the Windows cross-compilation #2713
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nurupo
force-pushed
the
update-windows-cross-compilation
branch
from
50693d4 to
d450019
Compare
iphydf
approved these changes
Feb 26, 2024
iphydf
previously requested changes
Feb 26, 2024
robinlinden
reviewed
Feb 26, 2024
nurupo
force-pushed
the
update-windows-cross-compilation
branch
3 times, most recently
from
893664f to
8fa52af
Compare
nurupo
force-pushed
the
update-windows-cross-compilation
branch
from
8fa52af to
84db0f5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2713 +/- ##
==========================================
+ Coverage 73.06% 73.11% +0.05%
==========================================
Files 149 149
Lines 30517 30517
==========================================
+ Hits 22297 22313 +16
+ Misses 8220 8204 -16 ☔ View full report in Codecov by Sentry. |
nurupo
force-pushed
the
update-windows-cross-compilation
branch
from
f0f31a7 to
c396690
Compare
|
The CI is kind of weird, it only partially tests the changes I have done to the Windows cross-compilation docker in this PR.
Would be nice if 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 |
-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 Some post suggests that but then I get this error.I think I will be sticking with the 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 |
nurupo
force-pushed
the
update-windows-cross-compilation
branch
8 times, most recently
from
bdf1b28 to
773bbe2
Compare
The same hardening flags as the slated for the upcoming gcc's `-fhardened` sets, sans `-Wl,-z,relro,-z,now` as MinGW-w64's gcc doesn't support -z flags. This adds a dependency on libssp -- gcc's stack protector library.
We used to make the dll manually as we previously had 3 dlls: libtoxcore.dll, libtoxav.dll and libtoxencryptsave.dll, but for Windows we wanted them to be all combined into libtox.dll with all the dependencies included: libsodium, libopus, libvpx, pthreads, etc, to reduce the overall dll size and simplify linking. However, since CMake now produces a single libtoxcore.dll with toxcore, toxav and toxencryptsave included, we don't have to do this manually anymore. This results in the dll being named libtoxcore.dll instead of the libtox.dll that it previously was, matching the static libtoxcore.a's name.
Including libwinpthread and libssp.
While fun utils are of very low quality, both code-wise and usage-wise -- not checking for failed mallocs, not offering usage instructions, etc., there are a couple of them that Windows users might find useful, like the vanity key generators or a savedata creator, for example. The building of the fun utils was broken on Windows due to the utils failing to find sodium.h, as no libsodium include dirs were set on the fun utils.
For some reason even with -DCMAKE_EXE_LINKER_FLAGS="-static" specified, CMake still links OpenMP dynamically, even though it links other libs statically, e.g. -pthread.
These programs link against libsodium, which already provides bin-to-hex and hex-to-bin conversion functions. Removing the misc_tools dependency shaves ~30KiB (in some cases 10%) off Windows static binaries using it.
A bit infuriating that when you submit a PR modifying the Windows cross-compilation script, the CI *builds* the current image in one job, but then *runs* an older (!) image from Dockerhub in another. So it tests that the modified image builds successfully but doesn't test if it runs successfully, instead testing if an older image runs successfully / testing if the current toxcore changes run successfully in an older image. This should build and run the same image with the current changes.
It's a disableable option since we allow the user to change versions of dependencies and we obviously have hashes only for the default versions, we are not able to verify hashes of any other version, so it might be handy to be able to disable the check in that case.
nurupo
force-pushed
the
update-windows-cross-compilation
branch
from
b22bb6d to
e742ded
Compare
nurupo
changed the title
|
nurupo
commented
Mar 4, 2024
There was a problem hiding this comment.
Reviewable status: 1 of 2 approvals obtained (waiting on @iphydf and @robinlinden)
.github/workflows/docker.yml line 166 at r4 (raw file):
Previously, iphydf wrote…
Nice :)
Done.
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. |
iphydf
approved these changes
Mar 4, 2024
robinlinden
approved these changes
Mar 4, 2024
nurupo
changed the title
Docker doesn't have an IPv6 network by default, so these tests were failing when using IPv6.
nurupo
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Wanted to bump the toxcore dependencies for Windows cross-compilation before the release, but ended up changing a lot more things.
This change is