Skip to content
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

Assorted fixes for Xcode 16 #10092

Closed
wants to merge 16 commits into from
Closed

Assorted fixes for Xcode 16 #10092

wants to merge 16 commits into from

Conversation

oskirby
Copy link
Collaborator

@oskirby oskirby commented Dec 3, 2024

Description

I've been trying to get my iOS development environment working again after the latest round of updates from Apple. Here's the fixes I've had to apply in order to get going.

The simulator even works too, but switching from device to simulator (and vice versa) is broken and requires a complete rebuild. Looks to be something to do with how sentry/breakpad gets built that bakes in the platform config. That probably needs to be turned into a fat binary or framework or something.

Reference

i.e Jira or Github issue URL

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

This option breaks the Qt/iOS application setup. Qt tries to override the
program entrypoint on iOS, and directs it to _qt_main_wrapper() instead
using a linker argument, but these arguments get dropped when building as
a dylib, causing Xcode to skip _qt_main_wrapper() and invokes main()
directly.
We encounter an assert in Xcode 16 because the sortCallback method
used in the DeviceModel does not satisfy strict-weak ordering due
to the special case added for the current device. To fix this we
can simplify the callback so that it sorts by creation time and
move the current device as a second step.
@oskirby oskirby marked this pull request as ready for review December 4, 2024 16:09
@oskirby oskirby requested a review from mcleinman December 4, 2024 16:11
Copy link
Collaborator

@mcleinman mcleinman left a comment

Choose a reason for hiding this comment

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

I'm getting this build failure when building for sim:

Building for 'iOS-simulator', but linking in dylib (/Users/mcleinman/code/mozilla-vpn-client/build-ios/qtglean/unified/debug/QtGleanBindings.framework/QtGleanBindings) built for 'iOS'

I'm happy to pair on this if you want - may be easiest.

Comment on lines 169 to 170
QString simDeviceName = QString(qgetenv("SIMULATOR_DEVICE_NAME"));
if (!simDeviceName.isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
QString simDeviceName = QString(qgetenv("SIMULATOR_DEVICE_NAME"));
if (!simDeviceName.isEmpty()) {
bool isSimDevice = !QString(qgetenv("SIMULATOR_DEVICE_NAME")).isEmpty;
if (isSimDevice) {

Nit: I find it more readable when it looks something like this (here and in other spots we check for sim). I'm a tad concerned a far-future reader will not immediately understand that this is checking just for iOS simulator (and not generally checking for iOS), but it's up to you.

src/models/devicemodel.cpp Show resolved Hide resolved
src/models/devicemodel.cpp Show resolved Hide resolved
@oskirby
Copy link
Collaborator Author

oskirby commented Dec 4, 2024

I'm getting this build failure when building for sim:

Building for 'iOS-simulator', but linking in dylib (/Users/mcleinman/code/mozilla-vpn-client/build-ios/qtglean/unified/debug/QtGleanBindings.framework/QtGleanBindings) built for 'iOS'

I'm happy to pair on this if you want - may be easiest.

I find that switching from device-to-simulator (or vice-versa) i have to delete the CMake build dir and start again. Once built for one, or the other, it becomes sticky and will only build that way.

I think the underlying cause is that static libraries we build ourselves (eg: the rust stuff, wireguard-go, and sentry) get the target platform baked into them and result in this kind of linker error. There are a handful of ways to tackle this:

  • We go out of our way to force the simulator to target x86_64, build device as normal targeting arm64 and then merge the simulator and device builds together as an Apple universal binary (this is the approach taken by Qt, by the way).
  • We build the library using native CMake (eg: add_library(libname <sources>) and rely on the Xcode generator to figure it out.
  • We build the library as a framework, which I think has some way to distinguish device vs. simulator builds for the same target arch.

@oskirby
Copy link
Collaborator Author

oskirby commented Dec 4, 2024

I dunno how much of a pain the simulator-switching experience really is but I figured the fix is too big of a thing to tackle in this PR, and at least it's less broken than before :)

@mcleinman
Copy link
Collaborator

My error was starting from a blown-away build folder for iOS - so starting from scratch. I suppose it wasn't a fully cleaned repo, I can try again with a fully clean repo.

Agreed that switching can be saved for another PR.

@oskirby oskirby requested a review from mcleinman December 5, 2024 16:51
@mcleinman
Copy link
Collaborator

mcleinman commented Dec 11, 2024

I'm still getting errors when building for simulator, when building the iosglean target:

Ld /Users/mcleinman/code/mozilla-vpn-client/build-ios/src/Debug-iphonesimulator/IOSGlean.framework/IOSGlean normal (in target 'iosglean' from project 'Mozilla VPN')
...
ld: building for 'iOS-simulator', but linking in dylib (/Users/mcleinman/code/mozilla-vpn-client/build-ios/qtglean/unified/debug/QtGleanBindings.framework/QtGleanBindings) built for 'iOS'
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

My steps:

  1. blow away the build-ios folder
  2. comment out the xlink lines in rustlang.cmake (as that was causing an error)
  3. run qt-cmake -S . -B build-ios -DCMAKE_OSX_SYSROOT=$(xcrun --sdk iphoneos --show-sdk-path)
  4. change the build settings to use the normal architectures
  5. build for an iPhone 16 simulator

I'm going to send you a meeting invite so we can look at this synchronously.

mcleinman and others added 9 commits December 12, 2024 12:56
This seems to be a bug in the daemon recovery logic. The initialization
steps starts with a QLocalSocket::abort() to clear the socket state and
this can sometimes generate a disconnect signal, treating disconnection
as an error can result in an infinite loop.
When running on mobile, we need to support a speculative onboarding
connection in the controller. This is a special case handled in the
iOS and Android controllers.

To try and generalize this, lets add an onboarding state and reason
to the controller and pass it down to the local-socket based daemons
so that they can setup permissions and emit a disconnection to end
the onboarding.
Copy link
Collaborator

@mcleinman mcleinman left a comment

Choose a reason for hiding this comment

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

I'm not sure how I feel about adding yet another state here. Let's all talk it through in our next engineering meeting? Or if you feel like this PR is getting stale, we can schedule a meeting next week if enough of us are around.

@oskirby
Copy link
Collaborator Author

oskirby commented Dec 23, 2024

I started this workstream with the intention of getting the iOS simulator working and it kind of got subsumed by trying to get builds working in Xcode 16. This has kind of grown to the point where it's no longer clear what the point of this PR is. I think the better path forward is to split this PR into two distinct chunks of work so that they can be reviewed separately.

@oskirby oskirby closed this Dec 23, 2024
This was referenced Dec 23, 2024
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