Skip to content

chore: set rust linker on win64, fix build crash#5345

Merged
kgutwin merged 2 commits intoPRQL:mainfrom
bioteam:kg/windows-build-fix
Jul 2, 2025
Merged

chore: set rust linker on win64, fix build crash#5345
kgutwin merged 2 commits intoPRQL:mainfrom
bioteam:kg/windows-build-fix

Conversation

@kgutwin
Copy link
Copy Markdown
Collaborator

@kgutwin kgutwin commented Jul 1, 2025

I did a little google searching of the recently appearing Windows build failures and found actions/runner-images#12432, which led to rust-lang/rust#141626 (comment). Let's see if this fixes the issue.

@max-sixty
Copy link
Copy Markdown
Member

bleh, no luck. we could just comment out the windows tests until it's fixed upstream?

@kgutwin
Copy link
Copy Markdown
Collaborator Author

kgutwin commented Jul 1, 2025

It did actually fix the linker-related crash, but it didn't fix the other issues :(

One is weird -- in .github/workflows/test-rust.yaml it successfully passes the "Compile" step using clechasseur/rs-cargo@v3, and you can see that it pulls in the insta package, but then it goes to run cargo insta test and complains "insta not found"? It doesn't really make sense.

The other one in test-js seems maybe that something (wasm-pack) was omitted from the runner image. I wonder if it's possible to add that back in somehow. Still looking into that one.

@max-sixty
Copy link
Copy Markdown
Member

It did actually fix the linker-related crash, but it didn't fix the other issues :(

oh nice! and the build now passes on windows

but yeah then the insta issue is surprising — it says it's installed: https://github.com/PRQL/prql/actions/runs/16012029427/job/45171587927?pr=5345#step:7:12

@kgutwin
Copy link
Copy Markdown
Collaborator Author

kgutwin commented Jul 2, 2025

Is the insta issue possibly related to the cache in some way? I wonder how to reset / clean out that cache, maybe it needs a fresh install for the latest runner.

I fiddled with the matrix settings for test-js and test-rust-main so that the failing Windows tests there won't stop the whole pipeline. Now, check-ok-to-merge is good with ignoring the failed Windows tests.

I'm going to go ahead and merge this now, since it's better than before. It's still worth trying a bit more to fix the failing tests though.

@kgutwin kgutwin merged commit d518251 into PRQL:main Jul 2, 2025
77 of 79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants