Use Win32 threads when compiling with Clang on MinGW #2381

ognevny · 2025-04-30T16:41:45Z

Issues:

Resolves #2362

Description of changes:

when compiling aws-lc with clang pthreads are currently used. it's common to build Clang itself with Win32 thread model so usually -lpthread option is not passed for *-pc-windows-gnullvm targets, for example. this PR fixes the issue when aws-lc-rs fails to build on such targets because required pthread symbols are not being linked in resulting binary

Call-outs:

-

Testing:

for now some Rust dependent software in MSYS2 is being built with this patch and there wasn't any runtime issue yet

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

codecov-commenter · 2025-04-30T19:07:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.77%. Comparing base (b0e8fbc) to head (fa43336).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2381      +/-   ##
==========================================
- Coverage   78.78%   78.77%   -0.02%     
==========================================
  Files         620      620              
  Lines      108077   108077              
  Branches    15349    15348       -1     
==========================================
- Hits        85153    85140      -13     
- Misses      22266    22280      +14     
+ Partials      658      657       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ognevny · 2025-05-01T02:35:45Z

CI failure seems unrelated

edit: oops, it should be MinGW specific

justsmth

Thanks for the PR! ❤️ I'll try to get a CI job setup to test this.

crypto/internal.h

ognevny · 2025-05-01T11:42:04Z

I'll try to get a CI job setup to test this.

you can try to setup llvm-mingw to get a minimal clang+mingw toolchain. a good example on downloading and using such toolchain is available in mingw-w64 mirror

justsmth · 2025-05-01T12:29:05Z

@ognevny -- I think I've almost got it all together... and it only took me a few dozen iterations. 🤣

The tests are failing and it's not clear why. (?) I plan to cherry-pick this commit onto your PR branch for testing.

ognevny · 2025-05-01T12:37:37Z

The tests are failing and it's not clear why. (?) I plan to cherry-pick this commit onto your PR branch for testing.

just my guess, but go toolchains are mixed. try to set GOROOT environment before CMake invocation. also you can use a perl from MSYS environment, it's located under /usr instead of /clang64. it's usually installed along with other packages, but to ensure it you can add perl: (colon is important!) in pacboy field

ognevny · 2025-05-01T12:44:12Z

locally ran tests and it also failed for me, but with an another error

[0/2] C:\WINDOWS\system32\cmd.exe /C "cd /D C:\msys64\home\maksa\forks\aws-l...m -shim-path C:/msys64/home/maksa/forks/aws-lc/build/ssl/test/bssl_shim.exe"Running SSL tests
go: downloading golang.org/x/crypto v0.10.0
go: downloading golang.org/x/sys v0.9.0
0/0/988/1000/5078
0/0/988/1000/5078
0/0/988/1000/5078
0/0/988/1000/5078
0/0/989/1000/5078
0/0/1988/2000/5078
0/0/1988/2000/5078
0/0/1989/2000/5078
0/0/2988/3000/5078
0/0/2988/3000/5078
0/0/2988/3000/5078
0/0/2988/3000/5078
0/0/2988/3000/5078
0/0/2989/3000/5078
0/0/3988/4000/5078
0/0/3989/4000/5078
0/0/4988/5000/5078
0/0/4989/5000/5078
FAILED (Peek-Basic-Server)
unexpected failure: local error 'read tcp [::1]:60434->[::1]:50601: i/o timeout', child error 'none', stdout:

stderr:


FAILED (Peek-ImplicitHandshake-Server)
unexpected failure: local error 'read tcp [::1]:60434->[::1]:50605: i/o timeout', child error 'none', stdout:

stderr:


FAILED (Peek-Shutdown-Server)
unexpected failure: local error 'read tcp [::1]:60434->[::1]:50609: i/o timeout', child error 'none', stdout:

stderr:


3/0/5078/5078/5078

exit status 1
FAIL    boringssl.googlesource.com/boringssl/ssl/test/runner    51.701s

FAILED: CMakeFiles/run_ssl_runner_tests C:/msys64/home/maksa/forks/aws-lc/build/CMakeFiles/run_ssl_runner_tests
C:\WINDOWS\system32\cmd.exe /C "cd /D C:\msys64\home\maksa\forks\aws-lc && C:\msys64\clang64\bin\cmake.exe -E echo "Running SSL tests" && cd ssl/test/runner && C:/msys64/clang64/bin/go.exe test -timeout 10m -shim-path C:/msys64/home/maksa/forks/aws-lc/build/ssl/test/bssl_shim.exe"
ninja: build stopped: subcommand failed.

ognevny · 2025-05-01T12:59:35Z

seems like you have to pass GOROOT env in each step... is there any cmake option to hardcode Go path?

ognevny · 2025-05-01T13:15:07Z

cmake itself uses different Go: -- Go compiler 1.21.13 found

.github/workflows/windows-alt.yml

justsmth · 2025-05-01T13:30:52Z

Test are passing: https://github.com/aws/aws-lc/actions/runs/14776095409/job/41484616153?pr=2381 😊

ognevny · 2025-05-01T13:33:10Z

great! how long will it take to integrate this change into aws-lc-sys rust crate? making it upstreamed ASAP will make maintenance burdens much easier... for now I have to explain how to fix the build

justsmth · 2025-05-01T13:40:44Z

great! how long will it take to integrate this change into aws-lc-sys rust crate?

After this is merged, we would need tag a release of AWS-LC then prepare a release for aws-lc-sys that aligns with it. We could get this all together by next week. I'll see if I can speed that up.

cargo manages to use vendored dependencies when they're listed with path also change style for rustup recipe a bit: '\t' -> ' ' and remove extra '/'

ognevny · 2025-05-04T04:56:10Z

is there any issue that should be addressed in this PR?

ognevny · 2025-05-05T06:12:09Z

is there any issue that should be addressed in this PR?

@justsmth ^

dkostic · 2025-05-05T21:10:34Z

There is a CI test failing, not related to the changes in this PR. I restarted the failed build and will merge the PR when it succeeds.

ognevny · 2025-05-06T02:57:44Z

There is a CI test failing, not related to the changes in this PR. I restarted the failed build and will merge the PR when it succeeds.

thanks, it succeeded now

@ognevny

### Description of changes: Prepare v1.51.1 ### What's Changed * Use Win32 threads when compiling with Clang on MinGW by @ognevny in #2381 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

To pull in aws/aws-lc#2381 which fixes build on MinGW with Clang Release Notes: - N/A

This is required to fix the build with clang+mingw on Windows, see aws/aws-lc#2381 Updated via "cargo update -p aws-lc-rs --precise 1.13.1" Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/merge_requests/2237>

ognevny requested a review from a team as a code owner April 30, 2025 16:41

ognevny force-pushed the fix-windows-clang branch from bee4e8e to 17dc891 Compare May 1, 2025 02:38

justsmth reviewed May 1, 2025

View reviewed changes

crypto/internal.h Outdated Show resolved Hide resolved

Use Win32 threads when compiling with Clang on MinGW

577ff2a

ognevny force-pushed the fix-windows-clang branch from 17dc891 to 577ff2a Compare May 1, 2025 11:39

CI job compiling for MinGW w/ Clang

75dbfb8

justsmth force-pushed the fix-windows-clang branch from 2abd797 to 74a11ba Compare May 1, 2025 12:54

justsmth force-pushed the fix-windows-clang branch 3 times, most recently from 99068d6 to 042297b Compare May 1, 2025 13:11

justsmth force-pushed the fix-windows-clang branch from 042297b to 807b7ae Compare May 1, 2025 13:18

ognevny commented May 1, 2025

View reviewed changes

.github/workflows/windows-alt.yml Outdated Show resolved Hide resolved

Update msys2; Fix Go

316cb30

justsmth force-pushed the fix-windows-clang branch from 807b7ae to 316cb30 Compare May 1, 2025 13:21

justsmth approved these changes May 1, 2025

View reviewed changes

Merge branch 'main' into fix-windows-clang

ead0ebd

ognevny referenced this pull request in msys2/MINGW-packages May 2, 2025

gst-plugins-rs,rustup: vendor all aws-lc-rs packages

3af71a3

cargo manages to use vendored dependencies when they're listed with path also change style for rustup recipe a bit: '\t' -> ' ' and remove extra '/'

dkostic approved these changes May 5, 2025

View reviewed changes

Merge branch 'main' into fix-windows-clang

fa43336

justsmth merged commit cddf0ec into aws:main May 6, 2025
114 checks passed

ognevny deleted the fix-windows-clang branch May 6, 2025 19:21

justsmth mentioned this pull request May 7, 2025

Prepare v1.51.1 #2398

Merged

This was referenced May 9, 2025

deps: Update aws-lc-rs to 1.13.1 zed-industries/zed#30332

Merged

deps: update aws-lc-rs to 1.13.1 rust-lang/rustup#4326

Merged

remove aws-lc-rs workaround; gst-plugins-rs: enable aarch64 builds msys2/MINGW-packages#24217

Merged

SomeoneToIgnore pushed a commit to zed-industries/zed that referenced this pull request May 9, 2025

deps: Update aws-lc-rs to 1.13.1 (#30332)

ea7756b

To pull in aws/aws-lc#2381 which fixes build on MinGW with Clang Release Notes: - N/A

ognevny mentioned this pull request May 20, 2025

deps: Update aws-lc-rs in lockfile rustls/webpki#355

Merged

Use Win32 threads when compiling with Clang on MinGW #2381

Use Win32 threads when compiling with Clang on MinGW #2381

Uh oh!

Conversation

ognevny commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues:

Description of changes:

Call-outs:

Testing:

Uh oh!

codecov-commenter commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ognevny commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justsmth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ognevny commented May 1, 2025

Uh oh!

justsmth commented May 1, 2025

Uh oh!

ognevny commented May 1, 2025

Uh oh!

ognevny commented May 1, 2025

Uh oh!

ognevny commented May 1, 2025

Uh oh!

ognevny commented May 1, 2025

Uh oh!

Uh oh!

justsmth commented May 1, 2025

Uh oh!

ognevny commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justsmth commented May 1, 2025

Uh oh!

ognevny commented May 4, 2025

Uh oh!

ognevny commented May 5, 2025

Uh oh!

dkostic commented May 5, 2025

Uh oh!

ognevny commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ognevny commented Apr 30, 2025 •

edited

Loading

codecov-commenter commented Apr 30, 2025 •

edited

Loading

ognevny commented May 1, 2025 •

edited

Loading

ognevny commented May 1, 2025 •

edited

Loading

ognevny commented May 6, 2025 •

edited

Loading