Now would be a good time to pull our changes :-) #18

Closed
wants to merge 85 commits into
from

Conversation

Projects
None yet
7 participants
@uliwitness
Contributor

uliwitness commented May 2, 2011

Here's what this branch does:

  • Removed dangling pointer with NSTimer
  • appcast entries with a "new version" but without a download (paid updates, update notification for users of dropped HW)
  • MacBook Air & Mac pro machine names
  • auto-size update alert & buttons
  • Borderless, fixed-size web view if desired.
  • Cope with screwed-up Application Support permissions
  • XIBs instead of NIBs
  • delegate method before/after alerts
  • localizations (XIBs only) - when in doubt I chose our localizations, since we pay professional localizers.
  • symbolic constants for machine types
  • Version number parsing that can cope with build numbers in brackets at the end
  • No more framework-style includes for people who want to compile in app instead of using the framework (easier debugging, no surprises if some other app installs Sparkle in /Library)
  • download on desktop/in trash on failure
  • installation now happens in the finish_installation tool (formerly relaunch) so we don't perform surgery on the app's open heart
  • More use of fileSystemRepresentation when dealing with C-string paths
  • misc fixes to authorized copying code.
  • mdimport no longer runs async
  • download's version, not host version in update alert
  • SULog for an easier post-mortem if an installation fails.
  • Reset the application package to the real name when installing. So users don't have an icon labeled MyApp 1.0 that is actually 3.0 (ifdef-controlled).
  • Don't have update notification float on top unless we are a faceless app (so users can set aside the window and get back to it).
  • More performSelectorOnMainThread: where needed to avoid threading-related crashes.
  • Permit automated downgrades (ifdef-controlled - we have a URL scheme that offers to downgrade, good for support to hand out)
  • Customization of how version numbers are displayed to the user.
  • Fix a possible crasher where a button wasn't disabled in some circumstances and could be clicked a second time.
  • ifdef to allow installing non-signed updates.

You can probably ignore all the ThreadSafePreferences stuff. Also, we do not use the stuff for transmitting system information in EyeTV, nor the RSA and delta stuff, so I recommend you have an eye on anything that may affect those, and run whatever test cases you have for these. We also localize using a database-based tool, so we don't use the .strings files. So when in doubt, choose our localized XIBs over the strings files.

This code (before the merge) has been in use for years with lots of users on our end, so is rock-solid. However, I may have caused the odd regression by merging. Feel free to ask if you have questions, here, on Twitter, or via e-mail.

uliwitness and others added some commits Dec 4, 2009

- More WIP creating finish_installation that waits
for quit, updates and relaunches the app, so
updates don't happen while app is still running.
NSBundle gets royally confused and points to the
old path (loading new NIBs into the old app in the
worst case) when you move a running app.
- Temp folder is now user's desktop, as
NSTemporaryDirectory() doesn't stay the same
between two separate apps, and may be emptied on quit.
- Relaunch gets created in App Support now, for
same reasons as above.
- Less dependency on prefix headers, that only leads
to lazy, un-reusable code.
Merge of changes from SVN repository:
- Changed NTSynchronousTask to also give the status return value and direct stderror output to the outputData.
- Changed includes so this builds as part of an app, too, not just as a framework
- Made sure SUAppcast's dealloc releases some leaked ivars.
- Added infoURL, extracted from link, that can point to a "more Info" page for download-less URLs
- Added support for version attribute on item so we can support update notifications that don't include an enclosure (e.g. paid upgrades, or upgrades that would require a system update)
- Added/improved a few description methods to ease debugging.
- Added SULog so one can ask for a special log with additional information when there are update issues.
- Added mayUpdateAndRestart for apps that absolutely, positively can't restart right now (e.g. cuz they're burning a CD and would produce a coaster).
- Added updaterWillRelaunchApplication delegate method, analogous to the notification. Useful to have app delegate quit helper apps during installation.
- Made SUBasicUpdateDriver's abortUpdate implicitly retain/autorelease the update driver, because the notification center otherwise releases it and it goes away, causing crashes in superclass's abortUpdate.
- Merge of SUKeepDownloadOnFailedInstallKey and SUFixedHTMLDisplaySizeKey.
- Avoid a few warnings about missing prototypes
- Be paranoid, hdiutil can verify the download again, so let it. Better for internal apps where we turn off DSA checks, too.
- SUHost has an -installationPath now, independent from the bundlePath, so one can normalize the app name from "MyApp 1.1b4" back to "MyApp" Users assume the file name contains the correct version number when there is one in it. Saves support a few round-trips each time.
- Be better at threading: Try calling non-thread-safe methods on main thread only, and don't assume delegates know when they need to be thread-safe, call them on main thread where possible.
- Added a method to put the old copy of the app in the trash. 1.5git changed in this spot, so I didn't actually merge the code that uses it back in yet.
- Fix version comparison so it doesn't get confused by bracketed build numbers in version strings
- Make sure cancel button is disabled during extraction, otherwise user would crash.
- Don't put auto-update window at floating window level. It's huge and can't be switched to background! If you're an NSBGOnly where you need that, turn it on only in that case, but don't generally do such nonsense.
- Hide release notes view if there aren't any.
- Test whether we are on dial-up before checking for updates in background. It's not nice to cause (possibly expensive) dial-up periodically.
- Temporarily comment out DSA complaints for easier testing.
- Don't store (possibly already invalidated) one-shot NSTimers in an ivar. It's bad style. Retain it instead.
- Decompress some monster expressions with nested method calls in ternary operators and nested in method calls again.
- Don't use implicit "id" for params or return types.
- finish_installation now puts up a progress window, so user knows update is still not finished.
- Use ThreadSafePreferences (included dummy version that uses regular prefs for projects that don't use ThreadSafePreferences).

- Todo later: Change finish_installation to be prettier.
Bring window to front when progress panel changes to "Install and res…
…tart", it's essentially a new alert coming up.
Added preprocessor defines to SUConstants.h for turning on/off DSA an…
…d downgrades. We let users downgrade using our URL scheme, and we warn them beforehand, so the downgrade possible attack paranoia gets in the way.
- finish_installation is now a real bundled app, so it can be localized.
- It contains its own copy of Sparkle.strings and SUUpdateStatus.nib so it can show its progress prettier
- We call TransformProcessType() before our first call to [NSApplication sharedApplication], so we can be launched as a command line tool and get a proper window server connection, and so our progress bars animate.
- We still support the old "relaunch"-style mode of operation, just by not passing the folder to install. This is useful for other relaunch occasions, like setting one's own dock icon flag.

- To do: Make sample app use the new finish_installation.
Build for more than just current architecture. We only build 32-bit b…
…ecause otherwise we'll get errors against 10.4 SDK (where 64-bit AppKit is not yet supported).
We should now correctly replace renamed apps, trashing the custom-nam…
…ed one, installing a new one with a default name, and also moving out of the way any existing copy with the default name.
We now really put the old app in the trash (and any file we have to m…
…ove out of the way to install a new app when we reset an edited name), even on non-admin user accounts. +++ TODO: Work around double authorization.
Uli Kusterer
Delegate can now control how version numbers are formatted/displayed …
…to the user, so we can e.g. exclude build numbers or whatever makes sense for a particular application.
Uli Kusterer
Set floating window level on the update alert again, but only for bg-…
…only apps. Don't want to annoy users by having an always-visible panel.
added Prefix header to finish_installation_tool
added SystemConfiguration.framework to Sparkle
added SUWindowController.m to finish_installation_tool
added SUStatusController.m to finish_installation_tool

now compiles Sparkle. Now going to fix the testbuild
Updating an application now longer shows the installation application…
… n the Dock (LSUIElement) , Removed TransformProcessType call as it will force the installer application to show in the Dock.
Make sure we delete the temporary folder when the user cancels the do…
…wnload. Also make sure we delete it on 10.4, which seems to have a bug in removeFileAtPath: that won't delete nested empty folders. As a quick fix, I just trash the folder in that case. We already put the old version in the trash anyway, and this saves me from having to write, test and debug a recursive removal function.
Be paranoid and wait for the mdimport to finish before we launch the …
…app. Might make spurious "no executable" errors a bit less likely.
updated the installer program to show an icon when the installation t…
…akes more than 1.2 seconds,

long installations have a program in the Dock
short installations just look like the program is restarting
some variable renaming to no longer have camelCasePath for char *
@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

I really don't want to put these downloads on the desktop. Let's put them in the application support folder instead.

I really don't want to put these downloads on the desktop. Let's put them in the application support folder instead.

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

We should be finding these paths with NSSearchPathForDirectoriesInDomains.

We should be finding these paths with NSSearchPathForDirectoriesInDomains.

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 6, 2011

Hm. But then even in the common case (update success), a user with an empty desktop sees something appear there for a while.

How about ~/Downloads (creating it if necessary for 10.4 folks)?

Hm. But then even in the common case (update success), a user with an empty desktop sees something appear there for a while.

How about ~/Downloads (creating it if necessary for 10.4 folks)?

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 6, 2011

Owner
Owner

uliwitness replied May 6, 2011

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 6, 2011

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 6, 2011

Owner
Owner

uliwitness replied May 6, 2011

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

We should be finding these paths with NSSearchPathForDirectoriesInDomains.

We should be finding these paths with NSSearchPathForDirectoriesInDomains.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

I'd like to inline these #defines with #ifndefs, so the Elgato branch can still override them, but I don't think we should have references to EYETV and TOAST in the master Sparkle branch.

I'd like to inline these #defines with #ifndefs, so the Elgato branch can still override them, but I don't think we should have references to EYETV and TOAST in the master Sparkle branch.

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

!!! I'm assuming this is removed in a later patch.

!!! I'm assuming this is removed in a later patch.

This comment has been minimized.

Show comment
Hide comment

Yep, fixed in d0b802e.

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner

Ah. You're right, it has the define.

Owner

uliwitness replied May 4, 2011

Ah. You're right, it has the define.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Ah, this is addressed in 5a5b171.

Ah, this is addressed in 5a5b171.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Need to handle the case of LSUIElement sorts of apps.

Need to handle the case of LSUIElement sorts of apps.

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

This comment has been minimized.

Show comment
Hide comment

Handled in e9c6967.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

This doesn't seem to be used anywhere.

This doesn't seem to be used anywhere.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

To be removed.

To be removed.

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

To be removed.

To be removed.

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

To be removed.

To be removed.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Will need to review the localization changes; as I recall the buttons are all made the size of the greatest button size.

Will need to review the localization changes; as I recall the buttons are all made the size of the greatest button size.

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Hm. Should this be in master?

Hm. Should this be in master?

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner
Owner

uliwitness replied May 4, 2011

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

… "the standard version displayer" …

… "the standard version displayer" …

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner

Ooops. Good catch. :-)

Owner

uliwitness replied May 4, 2011

Ooops. Good catch. :-)

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

I love the dynamic presentation here: short is invisible; long shows status. We do this all over the place in UIKit, and I think it's a great behavior.

I love the dynamic presentation here: short is invisible; long shows status. We do this all over the place in UIKit, and I think it's a great behavior.

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 4, 2011

Owner

It was a fight between Patrick and me whether we should show it or not. I think this was his idea.

Owner

uliwitness replied May 4, 2011

It was a fight between Patrick and me whether we should show it or not. I think this was his idea.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Contributor

I've reviewed most of this tonight. Didn't quite meet my goal of finishing the merge tonight, but not too much left to do. Only a couple small changes required, and only a small portion left the review. Localizations may need a fair amount of work to merge gracefully.

Uli, if you're coming to WWDC, you aren't allowed to pay for drinks as long as I'm nearby.

Contributor

andymatuschak commented May 4, 2011

I've reviewed most of this tonight. Didn't quite meet my goal of finishing the merge tonight, but not too much left to do. Only a couple small changes required, and only a small portion left the review. Localizations may need a fair amount of work to merge gracefully.

Uli, if you're coming to WWDC, you aren't allowed to pay for drinks as long as I'm nearby.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Ah, this should make the "lolz hueg boottun" localization issues go away. Excellent.

Ah, this should make the "lolz hueg boottun" localization issues go away. Excellent.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Unfortunately, this means that the user could potentially get several authentication dialogs. But that would take a great deal of work to correct, and this case should be rare.

Unfortunately, this means that the user could potentially get several authentication dialogs. But that would take a great deal of work to correct, and this case should be rare.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Hah! I did not realize that. Fantastic.

Hah! I did not realize that. Fantastic.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 4, 2011

Thank you so much for doing this legwork, Uli.

Thank you so much for doing this legwork, Uli.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 5, 2011

Contributor

I've reviewed these changes. Now, a few small modifications and testing. I won't have time tonight, but I'll do my best to land this ASAP.

Contributor

andymatuschak commented May 5, 2011

I've reviewed these changes. Now, a few small modifications and testing. I won't have time tonight, but I'll do my best to land this ASAP.

@uliwitness

This comment has been minimized.

Show comment
Hide comment
@uliwitness

uliwitness May 8, 2011

Contributor

On 05.05.2011, at 03:54, andymatuschak wrote:

I've reviewed these changes. Now, a few small modifications and testing. I won't have time tonight, but I'll do my best to land this ASAP.

Hi,

let me know if there's anything more I can do to help to help the changes get integrated.

Cheers,
-- Uli Kusterer
"The Witnesses of TeachText are everywhere..."

Contributor

uliwitness commented May 8, 2011

On 05.05.2011, at 03:54, andymatuschak wrote:

I've reviewed these changes. Now, a few small modifications and testing. I won't have time tonight, but I'll do my best to land this ASAP.

Hi,

let me know if there's anything more I can do to help to help the changes get integrated.

Cheers,
-- Uli Kusterer
"The Witnesses of TeachText are everywhere..."

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 8, 2011

Contributor

Thanks for the offer! I’m a bit swamped with work-work, but there’s not that much left to do for this, so I should be able to finish tonight or tomorrow night. I leave for a trip to France and Italy on Wednesday, so I really want to get it done before then! I don’t think I need help with anything in particular; just gotta dot the i’s and cross the t’s.

On May 8, 2011, at 3:57 AM, uliwitness wrote:

On 05.05.2011, at 03:54, andymatuschak wrote:

I've reviewed these changes. Now, a few small modifications and testing. I won't have time tonight, but I'll do my best to land this ASAP.

Hi,

let me know if there's anything more I can do to help to help the changes get integrated.

Cheers,
-- Uli Kusterer
"The Witnesses of TeachText are everywhere..."

Reply to this email directly or view it on GitHub:
andymatuschak#18 (comment)

Contributor

andymatuschak commented May 8, 2011

Thanks for the offer! I’m a bit swamped with work-work, but there’s not that much left to do for this, so I should be able to finish tonight or tomorrow night. I leave for a trip to France and Italy on Wednesday, so I really want to get it done before then! I don’t think I need help with anything in particular; just gotta dot the i’s and cross the t’s.

On May 8, 2011, at 3:57 AM, uliwitness wrote:

On 05.05.2011, at 03:54, andymatuschak wrote:

I've reviewed these changes. Now, a few small modifications and testing. I won't have time tonight, but I'll do my best to land this ASAP.

Hi,

let me know if there's anything more I can do to help to help the changes get integrated.

Cheers,
-- Uli Kusterer
"The Witnesses of TeachText are everywhere..."

Reply to this email directly or view it on GitHub:
andymatuschak#18 (comment)

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak May 10, 2011

Contributor

Merged with a number of modifications at https://github.com/andymatuschak/Sparkle/tree/ulimerge.

I'm asking for some community QA before I pull from there into master.

Hoorah!

Contributor

andymatuschak commented May 10, 2011

Merged with a number of modifications at https://github.com/andymatuschak/Sparkle/tree/ulimerge.

I'm asking for some community QA before I pull from there into master.

Hoorah!

@joshaber

This comment has been minimized.

Show comment
Hide comment
@joshaber

joshaber Jul 25, 2011

So... is this stuff pretty stable? Any idea?

So... is this stuff pretty stable? Any idea?

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak Jul 25, 2011

Contributor

I don't know if anyone's living on it. I have a suite of tests (not automated, sadly) I want to perform on it, but I'm too busy trying to get iOS 5 out the door now. :/

Contributor

andymatuschak commented Jul 25, 2011

I don't know if anyone's living on it. I have a suite of tests (not automated, sadly) I want to perform on it, but I'm too busy trying to get iOS 5 out the door now. :/

@ksuther

This comment has been minimized.

Show comment
Hide comment
@ksuther

ksuther Jul 25, 2011

Contributor

I've made a few more changes in my branch here: https://github.com/ksuther/Sparkle/tree/ulimerge

I fixed some build issues and deprecation warnings I ran into. The last commit changes the base SDK to 10.6 since I target Intel only, but isn't necessary.

Contributor

ksuther commented Jul 25, 2011

I've made a few more changes in my branch here: https://github.com/ksuther/Sparkle/tree/ulimerge

I fixed some build issues and deprecation warnings I ran into. The last commit changes the base SDK to 10.6 since I target Intel only, but isn't necessary.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak Sep 8, 2011

Contributor

I have been testing this branch. A couple issues discovered so far I'll need to address:
– finish_installation.app is displaying its UI unconditionally (i.e. without the hysteresis that makes it only display if the installation takes more than 1.2s)
– finish_installation.app restarts the host app, even if it is performing an "Install on Quit" (i.e. install only, no restart)

Contributor

andymatuschak commented Sep 8, 2011

I have been testing this branch. A couple issues discovered so far I'll need to address:
– finish_installation.app is displaying its UI unconditionally (i.e. without the hysteresis that makes it only display if the installation takes more than 1.2s)
– finish_installation.app restarts the host app, even if it is performing an "Install on Quit" (i.e. install only, no restart)

@rudyrichter

This comment has been minimized.

Show comment
Hide comment
@rudyrichter

rudyrichter Sep 29, 2011

Contributor

time to accept the pull request, andy.

Contributor

rudyrichter commented Sep 29, 2011

time to accept the pull request, andy.

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak Sep 29, 2011

Contributor

Rudy, this branch still has bugs that I am actively resolving. Just pushed one fix to the ulimerge branch tonight.

On Sep 28, 2011, at 9:07 PM, Rudy Richterreply@reply.github.com wrote:

time to accept the pull request, andy.

Reply to this email directly or view it on GitHub:
andymatuschak#18 (comment)

Contributor

andymatuschak commented Sep 29, 2011

Rudy, this branch still has bugs that I am actively resolving. Just pushed one fix to the ulimerge branch tonight.

On Sep 28, 2011, at 9:07 PM, Rudy Richterreply@reply.github.com wrote:

time to accept the pull request, andy.

Reply to this email directly or view it on GitHub:
andymatuschak#18 (comment)

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak Sep 29, 2011

Why the resource removal?

Why the resource removal?

@andymatuschak

This comment has been minimized.

Show comment
Hide comment
@andymatuschak

andymatuschak Oct 4, 2011

Contributor

I have landed this at last in 05861e5. Thank you for your patience and contributions, all.

Contributor

andymatuschak commented Oct 4, 2011

I have landed this at last in 05861e5. Thank you for your patience and contributions, all.

@danielpunkass

This comment has been minimized.

Show comment
Hide comment
@danielpunkass

danielpunkass Mar 20, 2014

Uli, can you take a look at line 364 and 365 above and let me know if my reasoning for why this should be changed rings true for you? It looks like the case this is handling is where the target file does not yet exist, so for example if we are trying to copy into /Applications/MyApp.app.

The test then for write access is confirming write access for both:

"/Applications"

AND

"/"

It strikes me that in the case where there is no file already there, it should ONLY need to test for write access to the container folder for the file to be created. I suspect the redundant check on the second-level parent is a vestige of copying/modeling on the code that handles the case where the file exists and thus permission for both the file and the containing folder needs to be tested.

The downside to the code as it stands in this commit is if the user DOES have access to the container folder, but DOES NOT have access to the second-level container, Sparkle will erroneously prompt for admin privileges to complete the installation.

AMIRITE? :)

Uli, can you take a look at line 364 and 365 above and let me know if my reasoning for why this should be changed rings true for you? It looks like the case this is handling is where the target file does not yet exist, so for example if we are trying to copy into /Applications/MyApp.app.

The test then for write access is confirming write access for both:

"/Applications"

AND

"/"

It strikes me that in the case where there is no file already there, it should ONLY need to test for write access to the container folder for the file to be created. I suspect the redundant check on the second-level parent is a vestige of copying/modeling on the code that handles the case where the file exists and thus permission for both the file and the containing folder needs to be tested.

The downside to the code as it stands in this commit is if the user DOES have access to the container folder, but DOES NOT have access to the second-level container, Sparkle will erroneously prompt for admin privileges to complete the installation.

AMIRITE? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment