Rewrite file operations for updating an app #575
Conversation
The improvements we want to achieve: * Stability - No more corrupt / incomplete updates by ensuring atomic move operations * Efficiency - Faster updates because of reduction of file copies that need to be done * Separation from Host and Helper - No usage of SUFileManager in the host application * Cleanness - Minimized deprecated APIs (no FSPathMakeRef, FSGetCatalogInfo, FSFindFolder, etc), use modern Cocoa APIs when possible, robust error handling, strong documentation, easier to read code. Many other small notes: * SUFileManager does not have class methods. Methods across an instance share an AuthorizationRef, instead of having to authorize every time a privileged operation needs to be done * The odd logic of `while (authStat == errAuthorizationDenied)` was removed * Instead of checking if we have write access, we try to invoke a normal operation first. If that doesn't work, with the right error returned back, we try authorizing, which is what Apple recommends doing * The trash is not used as a temporary location anymore. Rather we now request for a temporary directory to be created. This is more likely to succeed too (eg: on a drive that has no trash folder) * The old method for releasing the quarantine did not work until I used the "com.apple.quarantine" key instead of the LS constant that was being used. * For the old method for releasing the quarantine, we now check if a file has the quarantine xattr bit set before removing it. Just like how the new method works. * Quarantines can now be removed with authorization * The result of calling temporaryNameForPath: was never actually used in the old code * -[NSString fileSystemRepresentation] which can throw an exception is not used. Only getFileSystemRepresentation:maxLength: is used and the error on it is always checked * The desperate/hopeless check for if the code signature for the update is valid after the installation is complete is removed Potential issues that still needs to be looked in to: * Currently the method to move a file to the trash only works on 10.8+ * Appending of version number / picking destination name of app moved to trash
zorgiepoo
changed the title
|
|
||
| NSFileManager *fileManager = [[NSFileManager alloc] init]; | ||
|
|
||
| BOOL shouldAbortUpdate = NO; |
There was a problem hiding this comment.
Instead of using a state variable, could you move the code to a function, so if (!shouldAbortUpdate && becomes if ([self doSomeStuff] && [?
This code is fine, but I'm afraid such variables can cause trouble later if somebody moves the code around.
Since it is possible for the user to see these strings, we should not make them seem as 'gibberish' or unfriendly. More detailed error information is logged to the console.
|
Added commits to handle < 10.8 compatibility, appending the version to the older app's destination name, and making strings more user friendly. All work is pretty much done for review except for merging the recent |
I don't think this is actually needed, but better be safe than sorry.
| SUFileManager *fileManager = [[SUFileManager alloc] init]; | ||
|
|
||
| // Create a temporary directory for our new app that resides on our destination's volume | ||
| NSURL *tempNewDirectoryURL = [fileManager makeTemporaryDirectoryWithPreferredName:[installationURL.lastPathComponent.stringByDeletingPathExtension stringByAppendingString:@" (Incomplete Update)"] appropriateForDirectoryURL:installationURL.URLByDeletingLastPathComponent error:error]; |
There was a problem hiding this comment.
Since we're installing a single item, having a temp directory seems unnecessary.
Could you just move /tmp/foo.app to destination/foo (incomplete vX).app? or maybe to .foo (incomplete vX).app (with a . prefix to prevent the temp app from popping up in Finder)
There was a problem hiding this comment.
Creating a temp directory for this kind of operation is consistent with how some of the Cocoa APIs work. For example, the docs for replaceItemAtURL:withItemAtURL:backupItemName:options:resultingItemURL:error allows this, but wouldn't allow what you're suggesting. URLForDirectory:inDomain:appropriateForURL:create:error: may even do this as a last ditch effort. They are used for single files (e.g., documents).
Parking the temp directory in the same directory as the final destination is a last resort effort that won't be hit often. Of course we wouldn't want the user to see the incomplete directory visible, but if it's in a temporary location and the installation is interrupted, it may be cleaned out after a couple days or next reboot. Further, there might be some safety implications with having the temp directory be at an unpredictable/hashed location.
I'd rather use a provided Cocoa/System API for creating a temp file/directory where it's viable... I don't think this bit is much different from the old code, except it preferred using the trash instead of a temp location, which is worse.
There was a problem hiding this comment.
Plus I just realized immediatly parking at the final destination directory would mean we may have to shell out to privileged operations there for copying and quarantine removal, when it may not be needed. The less we have to shell out to privileged operations, the better.
By not taking account the rare case, we simplify the code by having less duplication. The tradeoff is that the rare case will be slightly less efficient.
| - (ssize_t)getXAttr:(NSString *)nameString fromFile:(NSString *)file options:(int)options | ||
| { | ||
| char path[PATH_MAX] = {0}; | ||
| if (![file getFileSystemRepresentation:path maxLength:sizeof(path)]) { |
There was a problem hiding this comment.
Wouldn't it be safer to use [file fileSystemRepresentation]?
There was a problem hiding this comment.
No, that can throw an Obj-C exception and doesn't return a copy.
Rewrite file operations for updating an app
|
Merged. Sorry for keeping it open for so long. |
|
It's cool since it's a big sort of change. I'll look into creating some unit tests for this class shortly. |
The improvements we want to achieve:
Many other small notes:
while (authStat == errAuthorizationDenied)was removedPotential issues that still needs to be looked in to:
Currently the method to move a file to the trash only works on 10.8+Appending of version number / picking destination name of app moved to trashThis is still a work in progress but most of the work is done. This may improve #441 and #558, and would resolve #498. This pull request is definitely a big change / should be reviewed carefully.