add android prebuilt binary support - #4729
Conversation
Android targets do need CGO_ENABLED=1 as it requires building some native C code. See upstream issue: golang/go#42725 (comment) Also move environment variable setting to getPlatforms(), as we need to set more architecture specific and os-specific environment variables now. Makes it much more cleaner to do it here. Also this means we are no longer setting GOARM=6 for architectures that don't need it, the variable should be accessed only for arm and arm64 targets, so setting it specifically only for them makes sense. Similarly set CC environment variable for cross-compiling for Android, and set CGO_ENABLED=1 when CC is set.
fanotify syscall requires CAP_SYS_ADMIN which is not granted without root on Android. Android terminates the process when the syscall is used instead of gracefully rejecting the syscall, so don't try to use it at all on Android
|
We're not going to have any of this NDK stuff on the build machine. I'm a bit confused why any of this is required, but maybe esbuild has been dealing with this too. |
For some weird unknown reasons cgo is not needed for android arm64 builds, and esbuild only does provide prebuilt binaries for android arm64. So they get away without using cgo entirely and having no dependency on Android NDK. I enabled cgo for arm64 Android as well in this PR for consistency across all the architectures. Also there is another issue without cgo where name resolution might fail due to Go trying to read |
|
If we want to do this, then I'm going to have to get NDK and so on on the builder, and I don't really want to do that until we move repos and get the pipeline in the same repo where it's easy to touch, so I don't think we'd want this PR at this point. |
Thanks that makes sense! Btw, I just did go through the codebase, and seems like tsgo isn't making any dns resolutions or any http requests that might trigger dns resolution (thanks to Gemini for the thorough search). I believe we can add support for Android arm64 now without cgo, and then once you move the pipelines over to here, I'll add support for other architectures. arm64 is most of active users according to deb download data I have for Termux. Would you be fine with it? |
Yes, this is super intentional 😄
After #4566, at least |
|
This might be slightly off-topic for this PR, but I was surprised to see that you guys are raw handling syscalls for fanotify and inotify on linux and darwin. Seemed quite contradicting considering you guys choose Golang for it being just native enough for being portable and you guys broke portability here. I guess that's cause Go doesn't provide any native way to do that. I used to follow TS dev blogs quite religiously back when Go experimental rewrite was first announced |
|
"portable" to us meant "we can port our code to it", because our TS was structurally similar to Go. Has nothing to do with platform specific code. |
I'm not highly experienced with Go, but I believe that as long as you end up building it in any way that causes the command then it will work properly. |
|
I don't think it's required; I am putting up another PR that's the simplified version of this which perhaps you all could check. But, it'll only work for arm64. |
Fixes #4715
This currently requires the Android NDK bin to be present in
$PATH. Tested using NDK r29. Had to modify theconst nativePreviewReleaseVersion = /** @type {string | undefined} */ (undefined);line inHerebyfile.mjsand hardcode it to a string in order to build for all targets at once, else it was only building for vsix targets.I believe upstream might be interested in explicitly using a certain version of NDK adding the toolchain bin in
$PATH, open to suggestions on that.Requesting explicit review from @jakebailey as they had assigned them the issue