-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Upgrade to the high-level ittapi v0.3.0 crate
#4003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I looked into this but don't know yet how to resolve it: the compilation failure is due to Locally I was unable to reproduce: after installing MSYS2 and compiling the I then attempted to reproduce on ittapi's CI: using a similar setup to the Wasmtime workflow, I added Any suggestions? |
|
@bnjbvr: since we can't really replicate this, we could just disable the ittapi dependency for MinGW builds? |
|
@abrown Need to check but I think that we do use this target to cross-compile internally. If so at least I might find people who could poke at this, maybe... |
|
Seems we don't use it, so yeah we could disable ittapi on mingw then, and if someone wants to support that, they would need to contribute a patch. Does it sound acceptable? |
It sounds acceptable to me, though others feel free to comment. I'm not sure how much energy to put into MinGW since I don't use it much (ever!) and I wonder sometimes if it has been obsoleted by WSL. |
|
Personally I find managing platform-specific exceptions and such to APIs not easy to maintain over time. To that end I would prefer to see this solved. It looks to me like MinGW doesn't provide the
That's because when you build an rlib it doesn't actually link anything, so this doesn't verify that symbols are actually present on the system.
If the
I don't think that the CI is configured correctly which is why this seems to work. In the logs I see: but if you're cross compiling the path to the binary should be but I don't think that works because the shell you're using is windows cmd or powershell (I dunno which) and probably doesn't use |
|
Very good call, @alexcrichton! Managed to reproduce the compilation error on |
In bytecodealliance/wasmtime#4003 we discovered that the `ittapi` C code uses the uncommon `strnlen_s` function. On older versions of MinGW (GitHub runners currently come with v8.1.0 pre-installed), this function was not available. Since v8.1.0, other versions have been published; this change re-installs MinGW with a recent version which includes `strnlen_s`. This commit has no code changes, just CI configuration to prove that the crate builds with the right MinGW version. As a result of this, we opened actions/runner-images#5530 to upgrade MinGW on all Windows GitHub runners. That upgrade may take some time so in the meantime the workaround is to use a new-ish version of MinGW. Note that this build issue is likely rare: it only really applies if someone builds the `ittapi` crate with `--target x86_64-pc-windows-gnu` AND with an old version of MinGW installed. Co-authored-by: Andrew Brown <[email protected]>
|
This should be merge-able once GitHub's virtual environment for Windows is updated with the latest version of MinGW: actions/runner-images#5729. |
|
I believe that the updated version of MinGW (which includes the missing |
|
I believe the issue here is that we're pinned to windows-2019 (see #3864 for info on that) so we're not getting updates. I'd reiterate though that there's no requirement to get this working on all platforms. Disabling this on mingw is fine. |
|
Just added a commit that disables this on windows + mingw, and it seems to compile fine. Unfortunately there's an unrelated MacOS build error (Github says the CI run has been cancelled), and I can't request a retry, so could anyone with the right authorization bits please do that? |
In bytecodealliance/wasmtime#4003 we discovered that the `ittapi` C code uses the uncommon `strnlen_s` function. On older versions of MinGW (GitHub runners currently come with v8.1.0 pre-installed), this function was not available. Since v8.1.0, other versions have been published; this change re-installs MinGW with a recent version which includes `strnlen_s`. This commit has no code changes, just CI configuration to prove that the crate builds with the right MinGW version. As a result of this, we opened actions/runner-images#5530 to upgrade MinGW on all Windows GitHub runners. That upgrade may take some time so in the meantime the workaround is to use a new-ish version of MinGW. Note that this build issue is likely rare: it only really applies if someone builds the `ittapi` crate with `--target x86_64-pc-windows-gnu` AND with an old version of MinGW installed. Co-authored-by: Andrew Brown <[email protected]>
This is using the new fancy
ittapihigh-level crate that we've worked on, @abrown @jlb6740 and myself, and it upgrades to the latest version.Could someone please test it locally and confirm it works for them? On my machine I get a weird failure that I also hit on main, not sure what's going on: