Skip to content

Fix Android support #22

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

colemancda
Copy link

Matches WASM implementation for locking, and disables building C++ code.

@Kyle-Ye Kyle-Ye requested review from Copilot and Kyle-Ye June 18, 2025 08:23
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR extends Android to follow the WASI build exclusions and stub implementations, and disables C++ code builds on Android.

  • Excludes Android alongside WASI for the Breakpoint publisher
  • Uses stub unfair lock and non-atomic identifier generation on Android
  • Removes C++ code targets (and the global C++ standard setting) for Android in Package.swift

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
Sources/OpenCombine/Publishers/Publishers.Breakpoint.swift Add os(Android) to the exclusion condition
Sources/OpenCombine/Helpers/Locking.swift Stub out __UnfairLock on Android, mirroring WASI behavior
Sources/OpenCombine/CombineIdentifier.swift Fallback to non-atomic combine identifier on Android
Package.swift Exclude Android from C++ helper targets and drop cxxLanguageStandard
Comments suppressed due to low confidence (1)

Package.swift:93

  • Removing the global cxxLanguageStandard may cause C++ helper targets on non-Android platforms to compile with an unintended standard. Re-add it or apply platform-specific settings to ensure consistent C++ builds.
    ]

@@ -9,7 +9,7 @@
@_implementationOnly import COpenCombineHelpers
#endif

#if os(WASI)
#if os(WASI) || os(Android)
Copy link
Preview

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stubbing out the lock on Android removes all mutual exclusion, which can lead to data races in multithreaded contexts. Consider providing a real lock implementation or documenting that this platform build is not thread-safe.

Suggested change
#if os(WASI) || os(Android)
#if os(WASI)
// Note: The WASI platform is assumed to run in a single-threaded environment.
// This implementation is not thread-safe.

Copilot uses AI. Check for mistakes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only be used on main thread in any case.

@@ -9,7 +9,7 @@
@_implementationOnly import COpenCombineHelpers
#endif

#if os(WASI)
#if os(WASI) || os(Android)
Copy link
Preview

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback for generating combine identifiers on Android is non-atomic and not thread-safe, which may produce duplicate IDs under concurrent use. Consider using an atomic counter or lock to ensure uniqueness.

Copilot uses AI. Check for mistakes.

@@ -90,8 +90,7 @@ let package = Package(
openCombineFoundationTarget,
openCombineDispatchTarget,
openCombineTestsTarget,
],
cxxLanguageStandard: .cxx17
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for this change. Could you elaborate on why this change is necessary?

Copy link
Author

@colemancda colemancda Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be some CPP interop issue with Android when not using the default setting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemancda Thank you for the explanation. However, I don't think relying on default settings is a good approach as it may introduce maintenance burden for future upgrades and it also breaks other platform's build currently (See the failure CI - https://github.com/OpenSwiftUIProject/OpenCombine/actions/runs/15523371994/job/44321270577?pr=22).

Here are my suggestions:

  • Test different cxxLanguageStandard explicitly: Could you test the Android build environment with different explicit cxxLanguageStandard values to determine which specific setting works? We can then use different explicit standards for different OS platforms if needed. If a higher standard is required, we can also upgrade directly and adapt the code accordingly.
  • Share the specific build errors: Could you please share the actual error messages you're encountering with .cxx17? This would help us determine if we can stick with a unified cxxLanguageStandard (e.g., .cxx17) and handle the compatibility at the code level instead.
  • Consider adding Android CI workflow: I'm not familiar with the current Android build environment setup. If possible, would you mind also adding an Android CI build workflow? This would help us catch such cross-platform issues earlier and ensure better compatibility going forward.

Relying on implicit defaults makes the build configuration less predictable and harder to maintain across Swift toolchain versions. Let's work together to find a solution that uses explicit settings while maintaining Android compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants