-
Notifications
You must be signed in to change notification settings - Fork 257
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
[BUG] NDK r28 beta1: issues with existing polyfills #2081
Comments
In gettext libintl, the mempcpy related errors also include:
|
Ooh, if vcpkg's CI has issues that gives us a great place to go look at the scope of the problem. Thanks! I should remember to try that whenever I'm making ambitious changes in the future. We'll have a look and see what can be done. My gut says that we'll re-hide the stuff that's causing problems for r28 and work on upstreaming fixes. If it were a shorter list of projects I'd say we should just fix those projects and ship r28 as-is, but if it's hit a non-trivial number of projects just in vcpkg, that's a lot of patches to upstream, and a lot of package updates that developers would need to take to get those fixes. A complete rollback is an option, but it's sort of the last resort. For the reasons mentioned in the changelog, this is a step in the right direction that we want to take, so I'd like to see us make progress wherever we can do so safely. I'm okay with gradual progress though, so if we need to re-hide the stuff that's broadly polyfilled already we can do that to avoid breaks while we fix the various open source projects to be compatible with the change. |
How did you do this? I was expecting to find a GitHub workflow for this, or instructions on how to build all the ports in their docs, but I'm not finding either. |
I eventually figured this out: |
This is very slow though so if there's a smarter way to do this please lmk. |
i agree with danalbert's plan to quickly fix all this by just adding ...but for the longer-term solution, here's a quick brain dump in case i go under a bus:
i don't really see any way to polyfill any of the unlocked stuff unless we re-expose the FILE implementation details. which maybe we should, but (a) that's a big decision and (b) we should definitely see what the wasm plan looks like in that area first. (since they might be the first actual beneficiaries of the increased opacity.)
large but doable, modulo similar FILE-internals leakage.
trivial one-liner. we should probably have done this as a polyfill in the first place and stopped there given that memcpy() can even be optimized to direct loads/stores, but mempcpy() is definitely going to cost you two function calls. where it should really just cost you an add.
so believe it or not, mblen() is a trivial wrapper for mbrlen() which is freely available. so surprisingly, this should be a trivial polyfill too.
this might seem like an obvious "no", but it's one where i wonder whether this is actually a great candidate for "let's just polyfill all the time". why? because Android has hard-coded answers to all the questions. and because every call i've ever seen asks a hard-coded question. so i'd expect that clang would be able to turn
the reliance on globals here makes this a "no".
even our limited set of supported encodings is probably too big for this to make sense.
as long as we don't care about not having the optimized arm64 implementation (which is kind of a moot point if your alternative is "nothing", as here), there's a trivial implementation of this.
certainly doable, though it's a weak symbol so that native bridge can override it, so doing so would likely cause a bit of trouble there.
nightmare.
this doesn't seem to touch any internal pthread state, and it's all relatively self-contained and based on <stdatomic.h>, so i think this is something we could do ... it would leak the currently-private internal implementation of pthread_barrier_t, though i suspect we could always cheat there by giving the polyfill struct and functions slightly different names?
conceptually easy, but really annoying as long as we're still supporting ILP32/32-bit off_t. which, realistically, is something we'll be doing for longer than we're still supporting API 24. certainly doable though. just gross.
getentropy() relies on getrandom() under the covers, and although that's been around since 3.17, it was only added to bionic at the end of 2017, so only 2018+ devices will have a seccomp policy that will let you use these. TL;DR: not likely. |
It a draft PR, microsoft/vcpkg#41293. It is in the timeline here. The failure logs are available as "assets" from Azure Pipelines.
Basically yes, but vcpkg CI also skips known failures with
Of course ... vcpkg builds debug+release, and you start with empty caches for assets and artifacts. You can look at individual ports with Maybe you can limit the build to the release config with |
I definitely looked for PRs... I must have accidentally done that in the wrong tab with a different repo. Thanks.
Ah, okay. If this is just a thing that's slow, alright. |
Note that when the Microsoft team added Android, they didn't fix all the existing ports. scripts/ci.baseline.txt really must be applied. Otherwise you will meet ports which will make you cry. |
Oh, I'd misunderstood you before and thought that was applied automatically. That explains why I've found mostly stuff that didn't work with r27 either. |
Bug: android/ndk#2081 Test: vcpkg install --triplet arm64-android gettext-libintl Change-Id: Id2eff76b929543f18d64accd5b6afb64bc63e117
I see (and have fixed) the |
No, this is more broken than you'd think. The I'm honestly not sure if I'm doing anyone any favors by "fixing" the NDK to make this port compatible again. I'm not convinced that the previous port which built would actually work. |
nothing says "Simple Authentication and Security Layer" like "we copied and merged multiple libcs" :-) |
I think this interpretation needs some adjustment, even if I didn't check gsasl in particular. Gnulib is not a standalone lib, but vendored into the configuration and build of software packages. It is organized into "modules", some meant to add, rectify or enhance implementations of ISO C or POSIX functtions. I don't consider myself an expert for autotools or gnulib. I honestly also don't know what is the best way to deal with this, and I don't ask for a particular solution. Gnulib providing an implementation under the same name may be hard to align with the Android ecosystem's variation over API level and actual runtime. The vcpkg problem is that the modules are littered into all those packages based on autotools. They all need to be patched individually until Gnulib improvements are integrated by each single package. |
Ah, that comment in the headers is pretty misleading if so. Odd. https://www.gnu.org/software/gnulib/manual/gnulib.html#Support-for-ISO-C-or-POSIX-functions_002e says that gnulib's polyfills are supposed to be prefixed to avoid exactly this kind of name conflict.
Yep, we do understand that. That's why we're partially reverting bionic's header changes for now. I am going to look through the existing ports while I do that though, because some of them may be problems with the ports. 7zip wasn't in your list, but it's the first one I looked at and it's just wrong, so soon 7zip should build for android: microsoft/vcpkg#41721. For the rest where it's not a port problem, I'll revert the bionic header change that's causing the problem. Like I said that's already been done for |
Ah, because I'd come to this bug prepared to find issues with polyfills, but that's not what this is. This looks to be an autoconf detection issue. It's trying to use |
I happened to be watching the log re-populate when rebuilding libuv after hiding the
It links though, so I guess those calls are probably culled by dead code elimination? |
Once this stack of commits is merged, the list you gave above all builds, aside from poppler, which is broken for other reasons. I think upstream forgot an It's going to be a bit before I can actually pull this update into the NDK. The OS repo is not in a state where I can pull a new sysroot, so I have to wait for that before I can update the NDK. We won't ship r28 without that, it's just going to be a while before we can get even a canary build to recheck vcpkg :( |
Description
This is in response to the r28 beta1 release notes.
A test build in vcpkg CI (which uses android 21) shows a lot of breakage related to
__INTRODUCED_IN()
with polyfills from gnulib. There is no full picture yet because the build error cascade starts early. The first hit ismempcpy
in GNU libiconv. I added a patch to rename the polyfill, but the next failures is already gettext-libintl.In the non-gnulib group, openssl is affected, removing many ports from the build. For poppler, I assume that GNU libiconv used to be the polyfill, but I didn't verify that. "libiconv as a polyfill" might be a broader pattern.
I grepped the arm64 logs for
__INTRODUCED_IN
. Transformed and compressed:Patching gnutext-libintl would probably add many more.
(All those packages build with NDK r26. NDK r27 is still blocked by its bugs.)
Affected versions
r28
Canary version
beta1
Host OS
Linux
Host OS version
vcpkg CI image
Affected ABIs
arm64-v8a
Build system
Other (specify below)
Other build system
No response
minSdkVersion
21
Device API level
No response
The text was updated successfully, but these errors were encountered: