2.x UserDriver API Redesign

[bdkjones]

I have immense respect for Sparkle and I am positive there are implementation details and issues of which I am not aware. That being said, I think the 2.x Custom UserDriver API should be re-thought.

It looks like what happened is that Sparkle's own internal flow for presenting UI elements was exposed, as-is, as a protocol. This isn't really ideal. For example, we have these three nearly-duplicate methods:

- (void)showUpdateFoundWithAppcastItem:(SUAppcastItem *)appcastItem userInitiated:(BOOL)userInitiated reply:(void (^)(SPUUpdateAlertChoice))reply;

- (void)showDownloadedUpdateFoundWithAppcastItem:(SUAppcastItem *)appcastItem userInitiated:(BOOL)userInitiated reply:(void (^)(SPUUpdateAlertChoice))reply;

- (void)showResumableUpdateFoundWithAppcastItem:(SUAppcastItem *)appcastItem userInitiated:(BOOL)userInitiated reply:(void (^)(SPUInstallUpdateStatus))reply;

All three are required to be implemented by anyone adopting the protocol, but I frankly don't care if an update is already downloaded or resumable—it has no effect on the UI I'm going to present to the user. If I DID care about this information, there would be a much better, cleaner way to get it: have the core Sparkle update-checker inform me that an update is available and allow me to inspect a property of that update to see if it's already downloaded or resumable, etc.

Proposal:

Completely divorce "Sparkle the Update Checker" from "Sparkle the Update Presenter".

Here's how Sparkle should really work:

1. I init the Sparkle Updater and assign a delegate. During this process, I set the bundles, tell Sparkle the feed URL, etc. In other words, I configure the updater.

2. I tell the updater to go check for updates and notify my delegate when it's done.

3. The updater calls my delegate and says:
   A) I found an update. Here's the SUAppcastItem for it.
   B) I didn't find any updates.
   C) I hit an error finding updates: there's no internet connection

4. My delegate decides when and if I'm going to present any UI in response to these events. Sparkle should never care if I do or don't take action; it just tells me what's available.

5. When I decide to present a UI for an update, I pull properties off the SUAppcastItem such as release notes, whether the update is already downloaded, whether the user previously delayed installing this item, etc.

6. I call a method on the updater:
   A) Install this AppCast item.
   B) Skip this AppCast item permanently.
   C) Mark this AppCast item to remind the user later.

7. The updater sends its delegate a message: "Hey, I'm ready to install and relaunch now". But it doesn't just wait for a reply to that. Instead, it just stops and sits there until I call something like -installAndRelaunch.

In Short

The existing protocol is like this:

"It's time to show UI element X"
"It's time to show UI element Y, let me know what the reply is"
"It's time to show UI element Z, let me know what the reply is"
"It's time to close UI element X."
"It's time to close UI element Y."

I think a better structure is for Sparkle (the core updater) to simply check for updates, notify its delegate of the progress/results, and then wait to be told to do something.

This mixing of the core update-checker logic and the UI presentation logic isn't clean. It makes using Sparkle 2.x very tedious. And it's going to be a nightmare to transition to SwiftUI or Swift when the time comes.

Rather than ship the 2.x branch and then support it for a decade, I recommend we take the time to rearchitect the "separation of powers" now so that Sparkle is set up for decades to come.

[bdkjones]

And, obviously, for folks that don't want to create a custom UI or manage the presentation themselves, there would be a default fallback that shows UI just as Sparkle currently does. But the core "update checking" element of Sparkle would be separate from that "presentation layer" of Sparkle so that the presentation layer could be easily replaced at any time.

[bdkjones]

Also, I realize this is a super-opinionated post. It is in no way my intent to disparage the work that's been done or insult the choices made—I've shipped millions of lines of code; I understand how things evolve and why choices from long ago result in complex interdependencies today.

I'm simply suggesting that a 2.x release is an excellent chance to do the hard work of separating the core logic and presentation logic and paying down some of the technical debt accumulated since the early 2000s.

[bdkjones]

I used the term "delegate" in my original post, but I realize that the existing updater has both a "driver" and a "delegate" and that those are separate things. "Presentation Driver" may be a less-confusing description than "delegate" in light of this.

Basically, if I init the updater and provide my own Presentation Driver, Sparkle becomes just a headless update-checker that just notifies my driver when stuff is available or changes or progress is made, etc. If I pass nil for a Presentation Driver, Sparkle would use its default one and show UI as it does today.

[bdkjones]

The more work I do implementing a custom UI with the current Sparkle 2 API, the more I think a redesign is warranted. For example, consider this method:

- (void)showUserInitiatedUpdateCheckWithCompletion:(void (^)(SPUUserInitiatedCheckStatus))updateCheckStatusCompletion;

I gather this method means "Show the little window that says 'checking for updates' with a cancel button and if the user clicks the cancel button, call the reply block with a 'cancel' value. Otherwise, call the reply block at some point in the future after I call a few other methods on you."

But then the next method is:

- (void)dismissUserInitiatedUpdateCheck;

And the docs for that method tell me this is a good time to call the reply block that was passed with the FORMER method. I gather this method gets called when Sparkle is done checking for updates and either has one to show or there isn't an update available. But now I have to save the reply block from the first method somewhere AND keep track of whether I've called it or not.

This sort of pattern plays out over and over as far as I can tell. It's not good. There shouldn't be blocks and complex callbacks like this. The process should go:

1. I tell updater to -checkForUpdates. I show whatever UI I want to at this time.

2. updater calls me back (either via delegation or NSNotification) to tell me the result of the update check. (Update available, no update available, error occurred.)

3. I listen for that delegate call or notification and update/dismiss my UI as appropriate.

I don't have to tell updater that I closed my UI. It shouldn't care. Likewise, updater shouldn't be telling me to close UI; that's on me. updater's sole job is to find updates and tell me about them.

Maybe I'm missing something, but the current approach seems VASTLY more complicated and interwoven than it needs to be.

[bdkjones]

I also think it's a bad idea to allow all of these protocol methods to be called from any thread.

The documentation comments are misleading because, in general, the comment "This method can be called from any thread" means: "YOU can call this method on ME from any thread; I'm prepared to handle that." But in Sparkle's case, I think what the comments are trying to say is: "Sparkle will call this method on YOU from any thread, and you better handle that."

Folks aren't going to realize that and they're going to forget to wrap each method's implementation in a dispatch to the main queue, so you're going to get a lot of people who update UI from background threads, which will produce crashes—but not always reliably, which makes it tough to debug.

And, the "any thread" thing violates the spirit of this API. I mean, the whole reason we're using it is to do custom UI work, right? That means our implementations are going to need to be on the main thread, so Sparkle should take care of that for us—not force the users of the API to wrap a thousand things in dispatch blocks to the main thread.

[bdkjones]

I think the general issue is that Sparkle was originally designed to be a completely automatic, one-stop-shop for updating apps. The update-checking, update-presenting, and update-installing stages were all tightly coupled and interdependent.

If we want to expand Sparkle to allow more flexibility and customization, we need to divorce those three elements from each other and make them loosely-coupled.

Doing that now, before 10,000 apps adopt this new API, is a good idea. It's a ton of work, but it'll be worth it when the new API is super clean and super simple.

[kornelski]

That makes sense.

But there's one detail that makes Sparkle dependent on what the UI does: it needs to know if an update is aborted, so it can delete temp files and reset state. So you can't completely ignore the UI, you would need to reply when it's irrelevant/rejected.

I agree these methods should be merged. It could be showUpdateForAppcastItem:_ someObjectExplainingHowDidWeGetThere:_ responseBlock:^(installNowOrLaterOrSkip) and the second object would specify whether that's a critical update, downloaded or not, user-initiated, etc.

[bdkjones]

That’s pretty straightforward: if the app gets to termination and “updater” has not received a “install the update” command from me, it simply deletes temp files and clears state.

I see it as Updater having an update “staged” and ready to go. If I then tell Updater to re-check for updates, it can clear that staged one out and redo the whole process.

And, of course, I can still tell it to abort at any time, which will also trigger the clearing and state reset. But that shouldn’t be REQUIRED to keep Sparkle in a consistent state.

[bdkjones]

To be clear, though: I’m proposing a large restructuring of Sparkle’s core architecture and flow, not a few minor changes to this existing API.

I’m happy to help do that, but it would take more than just me.

The good news is that all of the really “hard” stuff Sparkle does would not be touched and can continue as-is. The downloading, extracting, verification, and installing code wouldn’t be modified.

What WOULD be modified is the way Sparkle reports each step to the “UI Presenter” layer and waits to take action.

[kornelski]

I agree. That's a useful refactoring. Sparkle started with a very coupled architecture where update state is driven by the UI, and it decoupling them more would make both processes easier to follow.

[bdkjones]

Is there anyone else that needs to be on board? You look like the primary maintainer for the last five years. I'm currently implementing the 2.x custom API in one of my apps and doing that will give me a good feel for the process Sparkle is currently using and how it can be improved.

Also, I did find one bit of future-proofing that made me chuckle: Sparkle uses an unsigned 64-bit integer for the download length of an update. This means it's prepared to handle an update that's 18.4 exabytes in size. For context, ALL OF GOOGLE is about 15 exabytes (https://what-if.xkcd.com/63/).

[kornelski]

Yes, I'm mostly keeping lights on on the project. I'm happy for you to lead the design.

As for 64-bit sizes: I wouldn't be surprised if someone tried to send a 4GB update sooner or later :)

[bdkjones]

Sounds good. It’s going to take me a while to get that going, but I’m happy to take a crack at it.

Should we consider using Swift? I’m fine either way, but I get the feeling that Apple is going to start releasing Swift-Only stuff at a more rapid pace in the future. Looking down the road 10 years, getting started on the migration to Swift might be a good idea.

If not, I’m happy to use ObjC. Swift ticks me off on a daily basis, anyway.

[kornelski]

I don't mind adopting Swift in the long term, but I suggest doing one thing at a time. I'm worried that it could snowball into a major rewrite, and take too long to ship.

[bdkjones]

Got it. I basically envision refactoring the core “updater” to have a “presentationDelegate” property. It will send the delegate a series of messages (much like the current 2.x API) but won’t be reliant on the reply blocks, etc. In other words, Updater will just inform the presentationDelegate of its current status and wait to be told what to do.

I plan to guarantee that the delegate methods are called only on the main thread, since that’s convenient for the API consumer.

I would not rewrite existing Sparkle classes in Swift, but I would consider using Swift for the new classes and protocols I’d be adding, if that makes sense. (Including the new, default “presentationDelegate” class.) I’d avoid using structs and swift-only features so that ObjC apps can still use the new 2.x API. One less thing to move to Swift down the road. But, if it’s an issue I can write it in ObjC instead.

[zorgiepoo]

Hi all, just stopping by and this caught my interest a bit. I agree greatly that the API design and architecture should be reviewed on that branch for anyone using it. Just for more context:

The branch went through several major iterations and the threading concerns in the user driver are a relic of supporting a case where the user driver & updater/scheduler could be in separate processes (via NSXPC). This worked out mostly badly and should be dropped/forgotten IMO, should be trivial to verify/ensure it's "all main thread only". It's also why all the methods return void, reply blocks only have one parameter, all outstanding replies need to be responded, see user driver header docs for more details.

For simplifying pain points which I agree there are, one should be aware of the different update states Sparkle (and this branch) can be in. There are different sort of questions and some different sort of responses presented to the user depending if the update has already been downloaded or not, or if the update has already started installing and is not cancelable, or if update is just informational and so on. It would be incorrect to present "download the update" if the update has already been downloaded or "skip update" if the update has automatically been waiting to install when the app quits without possible cancellation, and so on.

On topic of review other than UI (but of intermingled relevance), there was a great deal of architectural changing between the updater and the installer (agent/daemon) as well which involved multiple stages of message passing. I created a couple of documents in Documentation/ (namely Installation.md and Security.md) for things I've forgotten by now.. It's been a few years since I've looked back at this :).

[edit]: I also am not sure if I was too harsh to deprecate the SUUpdater shim for adoption despite its coupling and instantiation problems..

[edit 2]: Since I don't follow too closely here, feel free to email me if wondering why a particular thing is the way it was designed/written and if I can recall why, would probably be glad to respond.

[zorgiepoo]

Reading a bit more here, SUAppcastItem should also not be made as a mutable state of passing unrelated information like if the update has been downloaded. Personally I think different presentation methods is not a bad decision; wouldn't want to adopters to encourage globbing different presentations together in a single method that needs to do several boolean checks. Slightly subjective and may be fine either way.

There may be pain points with user driver managing changing state however across different methods and I think that may be interesting to review over.

There is also UI-linking decoupling consideration. For example, the sparkle updater is UI-free so it should not rely on a dependency on the standard user driver/presenter if it has a UI dependency, thus should not have a way to "use default presenter if supplier passes nil"; moreover, nullability types should be minimized when possible from API design point. I had also wrote a SPUStandardUpdaterController class that composed of the updater and user driver with sensible defaults analogous to 1.x SUUpdater.

[zorgiepoo]

Having the userInitiated: flag was also a bad design decision on my part :). I added it for my own app, which cannot disrupt the user of showing updates for non-user initiated checks. That app has automatic downloads enabled and basically defaults to SPUDismissUpdateInstallation, SPUInstallLaterChoice, SPUDismissInformationalNoticeChoice when !userInitiated otherwise it presents UI. Instead, there ought to be a separate mode/delegation for this if it's worth supporting. The app also uses updater:shouldAllowInstallerInteractionForUpdateCheck: to return NO for background scheduled checks (since it doesn't allow background updates if it disrupts the user in case of privileged installs).

[edit]: Well maybe it makes sense if one would ever envision an app to have a different UI based on whether an update was user-initiated or not. Not so much in the case for the absence of a UI though.

[bdkjones]

I spent some time looking at the Sparkle codebase and thinking about the re-architecture. I also completed an integration using the existing API for a custom UI, which helped me grok the flow. Here are my thoughts:

1. Sparkle’s codebase is FAR from modern. It has supported old versions of OS X, so that makes sense.

2. Sparkle’s architecture is insanely complex. It was originally designed as a non-customizable, all-in-one, automatic updater with tight coupling between the UI and business logic.

3. I don’t think pouring a ton of work into the existing architecture makes sense. The existing custom-UI API is functional, if not convenient or pretty. In short, I don’t favor throwing a coat of paint on the project.

4. I would like to re-architect Sparkle from the ground up, in Swift, with a modern deployment target of macOS 10.14+. I envision keeping core, well-tested units of the existing codebase (downloader, signature-checker, installing agent, etc.) and keeping those in Objective-c. Everything else (essentially anything an implementor would see or interact with) would become Swift. Along the way, we simplify the API, separate the UI from the core, and drop support for legacy stuff that Apple is now discouraging (package installers, .dmg, etc.).

5. Doing (4) naturally solves problems with mutable shared state, threading, etc.

6. I do realize this is a crazy amount of work. Far more than I originally proposed when I suggested we revisit this API.

7. I propose that we start by producing a small, core MVP of Sparkle in the new architecture and then port extra features such as delta updates, etc. after that core MVP is in place.

8. I don’t necessarily think this should hold up a 2.x release. It will obviously take time to produce and test this new version.

[zorgiepoo]

Be sure to carefully visit the 2.x branch before another rewrite (it was a rewrite architecturally). It already decouples the UI and business logic of the project. That is also a small part; architecturally it's otherwise significantly changed internally and needed to decouple more than just that. I'm not sure how many people that are using it are aware of the internal architecture and all the decisions made that they aren't exposed to directly, which is why I advised the above documents. Some decisions in how the installer works, while decoupled, for example is still related to how the UI API was designed. Swift is a so-so choice and doesn't inherently solve any threading aspect I'd think. (Again the threading concern brought up here was misunderstood; they are called from the main thread and ought to consider it a 'doc' bug now).

[bdkjones]

I love Obj-C, but the writing is on the wall: Swift is the future of Apple platforms. Swift annoys the hell out of me in many ways, but there isn’t really a choice to he made here, unless we want to rewrite everything again in five years or so. (I see it like the choice between using Carbon or Cocoa 17 years ago.)

I did plan to use the 2.x branch as the starting point. I do see that it enforces the main thread for almost all those API calls where the docs suggest they might be called on any thread.

Swift’s value types would help us solve shared mutable state issues.

[zorgiepoo]

For clarity it should all be main thread unless the user driver one passes in is a remote XPC object, which nobody should / want to do.

I agree with Kornel's take on Swift and prioritization here.

[kornelski]

I get your feeling of "this is a mess, let's start over clean". This feels like an easy fix, but for the project it's dangerous:

• We're still going to have lots of users on 1.x, unless they have clear and easy path to upgrade.

• We'll have unfinished and abandoned 2.x, which has some users already.

• We'll have 3.x which is new and not feature-complete. Long tail of users may not be able to upgrade to it due to system requirements, missing feature, or just won't want to spend time on redoing integration with a new API, even if the new one is nicer.

This may turn out to be a Python 2/3 situation.

v2 is 90% ready. It needs EdDSA PR's finished, and we could start moving people over to it. Then we could kill off 1.x. If you make API changes in v2 in roughly the direction you want, then you can polish and reimplement them in Swift in v3 later, offering more gradual migration path.

[bdkjones]

@kornelski I couldn't agree more. The problem is that I don't think I can make those changes without a large restructuring. That should absolutely not hold up 2.x, because having used this branch extensively now in one of my apps, it's a solid improvement over 1.x.

The API may not be ideal, but it is functional. I'd recommend that we verify that the SPUUserDriver methods are all guaranteed to be called on the main thread, then remove those "this may be called on any thread" comments (@zorgiepoo indicated they are no longer correct).

A Swift port of Sparkle would be a good V3 and that would be a great time to drop legacy stuff, simplify and rearchitect the project.

[zorgiepoo]

There are good points brought up here about this particular API.

• Making all methods main thread only
• Whether or not an auxiliary data object should be used (+ having userInitiated or not) in place of multiple call points. I'm iffy on this but it was good to bring up.
• If data flow can be improved in some of the methods (eg not saving callbacks) easily.

In my experience of touching most of Sparkle's architecture, it's not all that bad (although it's been a while). I think rewriting things should generally be done a small modular level, piece by piece, where there are understood benefits, and where the old code and implications are well understood. Dropping legacy support of things is not likely hard from an engineering perspective. Tough problems can often have necessary complexity and be language agnostic. Compatibility is an important concern as well.

[zorgiepoo]

All the concerns I see here are cleaned up recently, and not any significant overhauling changes were needed:

• APIs on main thread only
• Completion handlers without having to save them and use them in another method, more explicit cancellation
• Show update methods merged

I still think this approach with the showable protocol is right over an optional delegation one.