Wasmtime GitHub notifications bot (Dec 05 2023 at 08:12): Wasmtime GitHub notifications bot (Dec 05 2023 at 08:12):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.
Wasmtime GitHub notifications bot (Dec 05 2023 at 08:12):kayabaNerve requested pchickey for a review on PR #7632.
Wasmtime GitHub notifications bot (Dec 05 2023 at 08:12):kayabaNerve requested wasmtime-core-reviewers for a review on PR #7632.
Wasmtime GitHub notifications bot (Dec 05 2023 at 08:12):kayabaNerve requested wasmtime-default-reviewers for a review on PR #7632.
Wasmtime GitHub notifications bot (Dec 05 2023 at 16:40):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!
Wasmtime GitHub notifications bot (Dec 05 2023 at 17:29):kayabaNerve updated PR #7632.
Wasmtime GitHub notifications bot (Dec 05 2023 at 17:29):kayabaNerve commented on PR #7632:
Rebased :+1:
Wasmtime GitHub notifications bot (Dec 05 2023 at 17:56):alexcrichton commented on PR #7632:
Mind undoing the changes to the CI configuration to get CI unblocked here?
Wasmtime GitHub notifications bot (Dec 05 2023 at 17:58):kayabaNerve updated PR #7632.
Wasmtime GitHub notifications bot (Dec 05 2023 at 17:58):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.
Wasmtime GitHub notifications bot (Dec 05 2023 at 18:37):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 05 2023 at 19:46):alexcrichton commented on PR #7632:
I'm unfortunately not familiar enough with everything in play here to know how to fix the CI failure.
Wasmtime GitHub notifications bot (Dec 06 2023 at 09:14):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.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:10):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
Wasmtime GitHub notifications bot (Dec 06 2023 at 18:32):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...
Wasmtime GitHub notifications bot (Dec 06 2023 at 20:07):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.
Wasmtime GitHub notifications bot (Dec 06 2023 at 21:36):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...
Wasmtime GitHub notifications bot (Dec 06 2023 at 21:38):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
Wasmtime GitHub notifications bot (Dec 07 2023 at 04:05):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.opaths, yet those do still specify GNU and I don't have an immediate reference point. Just noting my in-progress debug commentary.
Wasmtime GitHub notifications bot (Dec 12 2023 at 04:33):ChrisDenton commented on PR #7632:
The
.drectvewarnings 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.
Wasmtime GitHub notifications bot (Dec 12 2023 at 04:36):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)
Wasmtime GitHub notifications bot (Dec 12 2023 at 15:25):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.
Wasmtime GitHub notifications bot (Dec 15 2023 at 20:39):kayabaNerve updated PR #7632.
Wasmtime GitHub notifications bot (Dec 15 2023 at 20:40):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.
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:01):kayabaNerve updated PR #7632.
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:03):ChrisDenton submitted PR review.
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:03):ChrisDenton created PR review comment:
run: "C:\\msys64\\mingw64\\bin" >> $env:GITHUB_PATHSurely?
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:12):kayabaNerve submitted PR review.
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:12):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...
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:12):kayabaNerve updated PR #7632.
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:15):kayabaNerve updated PR #7632.
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:16):ChrisDenton submitted PR review.
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:16):ChrisDenton created PR review comment:
No worries, I was just going through my notifications and happened upon this.
Wasmtime GitHub notifications bot (Dec 15 2023 at 22:18):kayabaNerve updated PR #7632.
Wasmtime GitHub notifications bot (Dec 16 2023 at 03:57):kayabaNerve updated PR #7632.
Wasmtime GitHub notifications bot (Dec 16 2023 at 04:03):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.
Wasmtime GitHub notifications bot (Dec 16 2023 at 04:03):kayabaNerve has marked PR #7632 as ready for review.
Wasmtime GitHub notifications bot (Dec 16 2023 at 04:09):kayabaNerve commented on PR #7632:
https://github.com/kayabaNerve/wasmtime/actions/runs/7229601178/job/19700662991 for the relevant CI run.
Wasmtime GitHub notifications bot (Dec 17 2023 at 18:32):alexcrichton submitted PR review:
Thanks for this!
Wasmtime GitHub notifications bot (Dec 17 2023 at 18:59):alexcrichton merged PR #7632.
Wasmtime GitHub notifications bot (Dec 18 2023 at 04:01):kayabaNerve commented on PR #7632:
Happy to! Thanks for your patience.
Last updated: Nov 22 2024 at 16:03 UTC