kayabaNerve opened PR #7632 from kayabaNerve:main
to bytecodealliance:main
:
Unfortunately, due to the C dependencies, I couldn't locally verify this builds.
I did use the GitHub CI to verify it builds and the tests passed, yet due to the size of the CI and the fact I on-purposely didn't add a new vet statement which made the CI self-cancel, I'm unsure this is completely valid at this time.
On the belief this is a valid patch, my question is how should vet be handled. Personally, I see no reason to trust the windows-sys 0.48 published by Microsoft yet not windows-sys 0.51 given its FFI bindings, not some original library. If an attestation the changes are as expected is needed, I'd ask someone who has experience doing said reviews and whose name actually means something to this project performs said review. If no such explicit review is needed, I'd be happy to update the existing vet attestation if informed how.
kayabaNerve requested pchickey for a review on PR #7632.
kayabaNerve requested wasmtime-core-reviewers for a review on PR #7632.
kayabaNerve requested wasmtime-default-reviewers for a review on PR #7632.
alexcrichton commented on PR #7632:
No worries about the audits, I've posted those to https://github.com/bytecodealliance/wasmtime/pull/7634 and you can rebase this PR on top of that when it merges.
Otherwise thanks for this!
kayabaNerve updated PR #7632.
kayabaNerve commented on PR #7632:
Rebased :+1:
alexcrichton commented on PR #7632:
Mind undoing the changes to the CI configuration to get CI unblocked here?
kayabaNerve updated PR #7632.
kayabaNerve commented on PR #7632:
Oh. So sorry. I did my CI changes on a branch, yet it appears a block from there slipped in here. Fixed.
alexcrichton submitted PR review.
alexcrichton commented on PR #7632:
I'm unfortunately not familiar enough with everything in play here to know how to fix the CI failure.
kayabaNerve commented on PR #7632:
This says jobs were fixed, not that any failed :thinking: The amount of jobs also had GitHub cancel it on my fork, presumably some DoS protection. I wonder if because I'm making the PR, it's using my personal line of trust instead of wasmtime's...
I believe rerunning 'failed' jobs will preserve the existing passes and may move a few more forward.
alexcrichton commented on PR #7632:
Ah you'll need to click "view details" in the entry above about being removed from the merge queue. Scroll down there and you'll find the failure
kayabaNerve commented on PR #7632:
Thanks for pointing that out. The short summary is just 7 successful, 12 skipped.
I see it has errors, including:
Warning: corrupt .drectve at end of def file
Yet no other error than a very generic 'linking failed'. I would've expected one of these to have "Error:" as a prefix for a specific missing entry, not just how various things are unrecognized.
Considering this has absolutely no changes to any build scripts, I think this may be a regression in windows-rs, yet I don't want to point a finger so quickly. I'll try do more research tomorrow or so...
alexcrichton commented on PR #7632:
The short summary is just 7 successful, 12 skipped.
Ah you can chalk this up to GitHub UI. What you're looking at is this PR's CI which is separate from the merge queue's CI. The PR's CI does not contain the failure, the merge queue does. This is because the merge queue is running more tests than the PR does. More information about this can be found here
Yet no other error than a very generic 'linking failed'.
I'm as baffled as you! I don't know what's going on here. It may be MinGW-specific, not sure.
kayabaNerve commented on PR #7632:
I did have a run pass for Windows, I believe the msvc one. I also see some commentary on MinGW having issues historically, though I believe solely within the context of cross compiling...
kayabaNerve edited a comment on PR #7632:
I did have a run pass for Windows, I believe the msvc one. I also see some commentary on MinGW having issues historically, though I believe solely within the context of cross compiling...
EDIT: msvc run passed on my repo: https://github.com/kayabaNerve/wasmtime/actions/runs/7097634507/job/19318164298
kayabaNerve commented on PR #7632:
It's trying to link against a MSVC lib. Out of all the specified libs in the CI, I'm unclear which is the MSVC. I don't see any .lib extensions.
My immediate concern is the
C:\\Users\\runneradmin\\.rustup\\toolchains\\1.74.0-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-gnu\\lib\\rsbegin.o
paths, yet those do still specify GNU and I don't have an immediate reference point. Just noting my in-progress debug commentary.
ChrisDenton commented on PR #7632:
The
.drectve
warnings are not actually relevant (see https://github.com/rust-lang/rust/issues/112368) but may be masking an actual error. The warning spam is fixed in gnu binutils >=2.40.
kayabaNerve commented on PR #7632:
https://github.com/microsoft/windows-rs/issues/2614#issuecomment-1684152597 suggests a newer binutils may resolve this. I'll look at what version the CI is using...
(thanks to @ChrisDenton for the context)
alexcrichton commented on PR #7632:
Thanks for the tip! Unfortunately though I don't actually know how to update gcc and/or binutils here. We use whatever's already on GitHub Actions builders, so we're not already downloading something with a version number to bump.
kayabaNerve updated PR #7632.
kayabaNerve commented on PR #7632:
I wasn't asking for help. I was noting it had to be done. The above commit may achieve it. I'll fix any issues with it in about an hour, as I have to step out immediately.
kayabaNerve updated PR #7632.
ChrisDenton submitted PR review.
ChrisDenton created PR review comment:
run: "C:\\msys64\\mingw64\\bin" >> $env:GITHUB_PATH
Surely?
kayabaNerve submitted PR review.
kayabaNerve created PR review comment:
Yep. This is a WIP and not something I mean to submit as ready. I'm just forced to debug via my fork's GH CI, and every update I push to it is being mirrored here.
If the notifications are bothering you, I'll do my further tests on a branch. This was opened prematurely and should at least be marked a draft...
kayabaNerve updated PR #7632.
kayabaNerve updated PR #7632.
ChrisDenton submitted PR review.
ChrisDenton created PR review comment:
No worries, I was just going through my notifications and happened upon this.
kayabaNerve updated PR #7632.
kayabaNerve updated PR #7632.
kayabaNerve commented on PR #7632:
I believe this one is finally correct, based on its validity on a branch running the presumed relevant subset. Apologies for any exposure to y'all for the variety of misc attempts I had to suffer through. While I could write a few paragraphs of oddities I encountered, the final solution appears quite compact and barely introduce further dependencies (due to not using any additional actions, using a package manager already present in the CI, yet yes, downloading the updated package via Msys's infrastructure).
Feel free to request I squash the commit history to tidy it up with a proper message. I'm unsure if the history or the cleanliness is preferred.
kayabaNerve has marked PR #7632 as ready for review.
kayabaNerve commented on PR #7632:
https://github.com/kayabaNerve/wasmtime/actions/runs/7229601178/job/19700662991 for the relevant CI run.
alexcrichton submitted PR review:
Thanks for this!
alexcrichton merged PR #7632.
kayabaNerve commented on PR #7632:
Happy to! Thanks for your patience.
Last updated: Dec 23 2024 at 12:05 UTC