Remove incorrect guard check for Android

[finagolfin]

This was added a couple months ago and when I pointed out it will fail, I was told it would be fixed by some upstream changes and here later. I haven't heard anything in that comment thread since, so let's at least get this fixed in trunk and 6.1 for now, and we can adjust to Bionic upstream whenever that change is in.

@hyp, let me know if you're okay with just fixing this for now, as I currently have to do on my Android CI.

[finagolfin]

@swift-ci please test

[finagolfin]

I took a look at @hyp's github profile and it shows no activity for the last two months. Since this is an obvious fix that he agreed with back then and I want to get it into 6.1 next, going ahead and merging.

[hjyamauchi]

@finagolfin I don't personally have the context around the past PR but It appears that our internal CI started to fail after this PR with the error:

D:/r/_work/swift-build/swift-build/SourceCache/swift-corelibs-foundation/Sources/Foundation/Process.swift:946:53: error: value of optional type 'posix_spawnattr_t?' (aka 'Optional<OpaquePointer>') must be unwrapped to a value of type 'posix_spawnattr_t' (aka 'OpaquePointer')

Any thoughts?

[hjyamauchi]

CC @compnerd

[finagolfin]

I took a look at your build log and this is the full context it shows:

swift-corelibs-foundation/Sources/Foundation/Process.swift:946:53: error: value of optional type 'posix_spawnattr_t?' (aka 'Optional<OpaquePointer>') must be unwrapped to a value of type 'posix_spawnattr_t' (aka 'OpaquePointer')
  944 |         var spawnAttrs: posix_spawnattr_t = posix_spawnattr_t()
  945 | #endif
  946 |         try _throwIfPosixError(posix_spawnattr_init(&spawnAttrs))
      |                                                     |- error: value of optional type 'posix_spawnattr_t?' (aka 'Optional<OpaquePointer>') must be unwrapped to a value of type 'posix_spawnattr_t' (aka 'OpaquePointer')
      |                                                     `- note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
  947 |         try _throwIfPosixError(posix_spawnattr_setflags(&

Looking into this more, it appears your CI is still using the older NDK 26b from Oct. 2023, whereas I switched my daily Android CI over to NDK 27 once it was released last summer, finagolfin/swift-android-sdk@606ccefea. That current LTS NDK 27 has a patch that allows posix_spawnattr_init() to accept a nullable pointer and initialize it anyway, making this guard() check obsolete.

My suggestion is that you upgrade your TBC CI to NDK 27, which is the current LTS NDK, as NDK 26 is no longer supported by upstream. If that's not possible yet for some reason, you can always apply this guard() check patch locally on your CI, to keep your CI compatible with the older NDK 26 until you're ready to update to NDK 27. I was unaware that you were still using the older NDK 26 or that it differed in this type signature till you just linked to your build log, as @hyp was a bit vague in that linked comment thread about why he wanted to keep this check.

[hjyamauchi]

@finagolfin

I didn't notice first either but if you look a few lines below, there are more errors with other functions like posix_spawnattr_setflags and posix_spawnattr_destroy in addition to posix_spawnattr_init and those don't have the _Nullable annotations and still have _Nonull annotations. So, without the guard, my local build still fails with r27c.

These are the declarations in /usr/include/spawn.h from r27c

int posix_spawnattr_init(posix_spawnattr_t _Nullable * _Nonnull __attr) __INTRODUCED_IN(28);
int posix_spawnattr_destroy(posix_spawnattr_t _Nonnull * _Nonnull __attr) __INTRODUCED_IN(28);

int posix_spawnattr_setflags(posix_spawnattr_t _Nonnull * _Nonnull __attr, short __flags) __INTRODUCED_IN(28);
int posix_spawnattr_getflags(const posix_spawnattr_t _Nonnull * _Nonnull __attr, short* _Nonnull __flags) __INTRODUCED_IN(28);

So it seems that we still need the guard with r27c. Is there a chance that your CI doesn't build these?

[finagolfin]

OK, I looked into it further and I forgot that I myself am using an even older version of these posix_spawn APIs from the Termux packages for Android, that predates these nullability annotations even getting added. I use that Termux header and library because I build against Android API 24 on my Android CI, one of the oldest APIs still supported when these posix_spawn APIs were not available in Bionic, so I had backported Bionic's posix_spawn implementation to API 24 back then and stuck it in that Termux package.

So it turns out both @hyp and I were using old versions of the Bionic spawn.h, his with older nullability annotations and mine without any! 😺

There are a couple fundamental issues that remain. This guard check is fundamentally incorrect, as it will always unwrap and fail at runtime, as I just confirmed by extracting this snippet and running it locally. All @hyp was doing was shutting up the typechecker in the compiler by adding this check, as it would check that the types were correct after unwrapping, but would then fail at runtime.

Are you TBC people running these Foundation tests for Android to make sure this executes properly? I suspect not.

In my case, not only do I periodically build this repo natively on Android, but I run its test suite too.

Based on the problems you're seeing with compiling against spawn.h with nullability annotations, clearly this Swift code doesn't compile correctly against those annotated Bionic APIs. But the way Alex was avoiding that compilation failure was wrong, simply trading a sure compilation failure for a sure runtime failure.

I'm open to ideas on how to fix this in this repo going forward. An easy way for you to fix this on your CI for now might be simply to add a patch commenting out all this failing spawnattr code locally, since you don't appear to be actually executing it, just compiling it. That will get your CI going again and give us time to figure out how to properly address these new nullability annotations.

I've been meaning to backport the latest version of these posix_spawn APIs from Bionic back to that linked Termux package for API 24, including the nullability annotations and some newly added APIs. When I do, I will submit a patch adapting this code too. If you TBC people want to try and submit your own nullability solution in the meantime, great, but this guard check was always incorrect.

[hjyamauchi]

Ah :)

I think the original, existing runtime behavior in this Foundation code on Android was for the posix_spawnattr* calls to fail and _throwIfPosixError throws the exception. So what #5024 didn't eliminate this runtime error but just retained the existing behavior if you look at what _throwIfPosixError does and but just made buildable. So it seems like an improvement though it's not as good as making it work at runtime. I'd not call it incorrect. So I'd vote for adding the guard code back. Do you object?

[finagolfin]

I think the original, existing runtime behavior in this Foundation code on Android was for the posix_spawnattr* calls to fail and _throwIfPosixError throws the exception.

Why do you believe that? As I pointed out, it runs fine with the earlier Bionic spawn.h from Termux that I linked, which has no nullability annotations.

I'd not call it incorrect.

No, it's clearly incorrect, as it always fails at runtime. Your argument is merely that the subsequent code is also broken at rumtime, so it's okay if you put this broken check before just to keep it compiling, but I think you're wrong about the subsequent _throwIfPosixError failing.

In any case, I still disagree with the premise of adding this clearly incorrect check, just to keep your CI building.

So I'd vote for adding the guard code back. Do you object?

I do. Nobody is stopping you from adding such broken code locally to your CI, but you shouldn't be doing so upstream here.