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

Update to application-services v73.0.0 #8044

Closed
wants to merge 1 commit into from

Conversation

rfk
Copy link
Contributor

@rfk rfk commented Feb 25, 2021

I'm opening this as a draft for early visibility.

The latest release of Application Services includes:

  • The first iOS version of the Nimbus SDK
  • New automatically-generated bindings for the FxA Client component

This bumps the version of AppServices in firefox-ios, and makes the necessary changes to accomodate the new FxA Client component API. The changes here are almost entirely cosmetic, due to things that were named differently between the Rust code and the hand-written Swift bindings, and are now named consistently thanks to the auto-generation of the bindings.

I haven't tried to expose any Nimbus code yet, but we'll need to get these FxA changes landed before trying to do so, in order to get up to the right version of AppServices.

@@ -924,7 +924,8 @@ open class BrowserProfile: Profile {
guard let self = self else { return }
let devices = state.remoteDevices.map { d -> RemoteDevice in
let t = "\(d.deviceType)"
return RemoteDevice(id: d.id, name: d.displayName, type: t, isCurrentDevice: d.isCurrentDevice, lastAccessTime: d.lastAccessTime, availableCommands: nil)
let lastAccessTime = d.lastAccessTime == nil ? nil : UInt64(clamping: d.lastAccessTime!)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stable Kotlin doesn't have unsigned types, so the autogenerated API bindings can't have unsigned types, so we need to do this weird type conversion here. Sorry :-(

msg "Building Application Services."
pushd "${APP_SERVICES_DIR}"
./build-carthage.sh --no-archive
popd
echo ""
msg "Copying local Application Services (${APP_SERVICES_DIR}) into Carthage build directory"
rsync -ad "${APP_SERVICES_DIR}/Carthage/Build/iOS/MozillaAppServices.framework/" "${FRAMEWORK_LOCATION}/"
Copy link
Contributor Author

@rfk rfk Feb 25, 2021

Choose a reason for hiding this comment

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

Since we've moved to calling carthage copy-framework on the appservices framework, this setup with the symlink no longer works - Carthage tries to be clever and dereference the symlink and gets upset that it's pointing outside of the build root. Having a little script here that can physically rebuilt and rsync to the right place still seems useful though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this into a separate PR at #8081

@rfk rfk force-pushed the fxa-client-uniffication branch 3 times, most recently from d68f5d8 to bfc27bc Compare March 10, 2021 05:44
@rfk rfk changed the title WIP updates to accommodate UniFFI-cation of the fxa-client crate. Update to application-services v73.0.0 Mar 10, 2021
@rfk rfk marked this pull request as ready for review March 10, 2021 05:45
@rfk
Copy link
Contributor Author

rfk commented Mar 10, 2021

In terms of QA, I've manually built firefox-ios using this release and tried it out locally, signing in to my Firefox Account and syncing and what-not. The only thing I couldn't successfully test yet was receiving sent tabs, it's not working for me locally but that's almost certainly because I don't know how to test that properly in the simulator. I'll try some more later.

@isabelrios
Copy link
Contributor

Hi @rfk we have set a process to test our builds with latest a-s tag in a-s repo. Using 73.0.1 there is a compilation error, please see logs https://app.bitrise.io/build/a8e71ad679432504.
I know this is to update to 73.0.1 but thought I should mention just in case...

Also for 73.0.0 using xcode 12.4 and iOS 14.4, looks like the build is failing with error:
https://app.bitrise.io/build/9e2a2dfd5a068dfa#?tab=log

❌ /Users/vagrant/git/RustFxA/RustFirefoxAccounts.swift:235:55: cannot convert value of type 'Avatar' to expected argument type 'String'
if let str = avatarUrl, let url = URL(string: str) {
^
❌ /Users/vagrant/git/RustFxA/RustFirefoxAccounts.swift:316:29: cannot assign value of type 'Avatar?' to type 'String?'
avatarUrl = profile.avatar

This release includes new auto-generated Swift bindings for the FxA Client
component, which come with a few breaking API changes. There should be
no changes in functionality though.
@rfk rfk force-pushed the fxa-client-uniffication branch from bfc27bc to 5fe4d4a Compare March 11, 2021 19:43
@rfk
Copy link
Contributor Author

rfk commented Mar 11, 2021

Also for 73.0.0 using xcode 12.4 and iOS 14.4, looks like the build is failing with error:

@isabelrios thanks! It looks like I didn't update Cartfile.resolved when I was making this PR, so the build was still using an older version of appservices. I've pushed the updated Cartfile.resolved but I guess maybe I need someone on the team to re-trigger the bitrise job, it doesn't seem to be re-running automatically.

@isabelrios
Copy link
Contributor

Oh that was an easy fix then :) Just re-triggered the job on Bitrise 🤞

@rfk
Copy link
Contributor Author

rfk commented Mar 12, 2021

Jeremy asked in slack about possible impact on binary size and startup time, so I had a bit of a poke around. The compiled framework for this release does seem to be substantially bigger than previous releases:

[rfk] ~/Desktop/TEMP % du -hs appservices-*/Build/iOS/MozillaAppServices.framework/MozillaAppServices
 36M    appservices-69.0.0/Build/iOS/MozillaAppServices.framework/MozillaAppServices
 36M    appservices-72.1.0/Build/iOS/MozillaAppServices.framework/MozillaAppServices
 46M    appservices-73.0.0/Build/iOS/MozillaAppServices.framework/MozillaAppServices

(IIUC that's the cumulative size for both platform and simulator code, but still...)

Some size increase here is to be expected because of the new Nimbus code, but it doesn't feel like it should contribute that much of a difference. I'll see what more I can find out about the source of the delta.

@rfk
Copy link
Contributor Author

rfk commented Mar 12, 2021

I'll see what more I can find out about the source of the delta.

A big chunk of the increase here seems to be due to our switch from Rust 1.43 to Rust 1.50, I've filed an issue at mozilla/application-services#3925 to investigate further.

If the size of the current release is going to be a problem, I'm afraid I'll have to lean on firefox-ios folks to help us understand what an appropriate target size is here. (And just to re-iterate, the sizes I'm looking at above include simulator code and other things that don't ship with the app, I don't know how much of that will flow through to an increase in the size of the final app).

@mergify
Copy link
Contributor

mergify bot commented Mar 25, 2021

This pull request has conflicts when rebasing. Could you fix it @rfk?

@rfk
Copy link
Contributor Author

rfk commented Mar 25, 2021

Looks like this was superseded by #8219, closing. Thanks all!

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