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

Min version #1574

Merged
merged 1 commit into from Sep 6, 2013
Merged

Min version #1574

merged 1 commit into from Sep 6, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 18, 2013

See the discussion on the dev groups

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 18, 2013

Ugh. release? 😃 Ok, I'll look it over.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 18, 2013

Yep :)

We still need to do this as well:

  • Transifex updates
  • Add t-shirt comp to the changelog

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 19, 2013

So, what will a user see on 10.6 now? Something helpful? Or will it just do nothing when launched?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 19, 2013

So, what will a user see on 10.6 now? Something helpful? Or will it just do nothing when launched?

Good question. I'd assumed there'd be a popup and stuff, but I just tried changing the min system version value in my Info.plist to 10.9 (I'm on 10.8 atm) and QS just crashed.
Perhaps @HenningJ could download build 4003 from http://qs0.qsapp.com/plugins/files/com.blacktree.Quicksilver__16387.dmg, change the Info.plist as in this pull request and report what he finds?

Thanks

On 19 Awst 2013, at 23:32, Rob McBroom notifications@github.com wrote:

So, what will a user see on 10.6 now? Something helpful? Or will it just do nothing when launched?

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 19, 2013

Yeah, I just tried it an get a crash as well. It appears to be by design, since the crash report says “application requires at least Mac OS X version 10.9.0, but is being run on 10.8.4, and so is exiting”. The same message appears on the Console for those that know to look there.

Should we leave it? Looks like the current code shows an obvious message.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 4, 2013

FYI, this (or some kind of fix) should still be integrated into release

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 4, 2013

I've come up with another solution which hopefully works - moving the checking code to run sooner rather than later.

@HenningJ - can you please check that if you build this branch and run, you get the dialog popup as opposed to an app crash? We'll need this for v1.1 :)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Sep 5, 2013

I'm not sure if I can actually still build anything. With all the >10.6
changes you all have made by now. But I'll try tonight.

On Thu, Sep 5, 2013 at 1:23 AM, Patrick Robertson
notifications@github.comwrote:

I've come up with another solution which hopefully works - moving the
checking code to run sooner rather than later.

@HenningJ https://github.com/HenningJ - can you please check that if
you build this branch and run, you get the dialog popup as opposed to an
app crash? We'll need this for v1.1 :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1574#issuecomment-23833621
.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 5, 2013

OK, if not I'll send you a built app :)

On 5 Medi 2013, at 16:34, Henning Jungkurth notifications@github.com wrote:

I'm not sure if I can actually still build anything. With all the >10.6
changes you all have made by now. But I'll try tonight.

On Thu, Sep 5, 2013 at 1:23 AM, Patrick Robertson
notifications@github.comwrote:

I've come up with another solution which hopefully works - moving the
checking code to run sooner rather than later.

@HenningJ https://github.com/HenningJ - can you please check that if
you build this branch and run, you get the dialog popup as opposed to an
app crash? We'll need this for v1.1 :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1574#issuecomment-23833621
.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 5, 2013

I'm assuming it won't build under 10.6, so here's a build from this branch. http://cl.ly/040C0u0M273n

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 6, 2013

I tested this by changing it to require 10.9+ and running it. I saw a nice friendly message, then the app quit. Looks about right to me.

skurfer added a commit that referenced this issue Sep 6, 2013
@skurfer skurfer merged commit dde484c into release Sep 6, 2013
@skurfer skurfer deleted the minVersion branch Sep 6, 2013
skurfer added a commit that referenced this issue Sep 6, 2013
@tiennou
Copy link
Member

@tiennou tiennou commented Sep 6, 2013

Hem, I'm not sure that will fly with dyld. I don't remember the exact thing this is trying to fix, but if dyld fails to find any classname when it binds symbols at startup, you will hit a system crash dialog no matter what, so the test on 10.9 isn't worth anything. Just my 2 cents ;-).

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 6, 2013

So this probably won't help in that case, but it won't hurt, right?

@tiennou
Copy link
Member

@tiennou tiennou commented Sep 6, 2013

Yep. As I said the correct way to have backward compatibility like that is to weak-link 10.7+ classes and check them at runtime. In that case, dyld will see the link marked as weak and will just nil the reference (eg. you can test it with NSWeakClass == nil, but since messaging to nil does nothing blah blah, you get the point ;-).

For selectors, it doesn't need anything else than a respondsToSelector: dance.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Sep 7, 2013

I tested the build Rob made for me. No nice error message or anything, just a silent crash:

Process:         Quicksilver [798]
Path:            /Applications/Quicksilver.app/Contents/MacOS/Quicksilver
Identifier:      com.blacktree.Quicksilver
Version:         1.1.0:minVersion (4004)
Code Type:       X86-64 (Native)
Parent Process:  launchd [101]

Date/Time:       2013-09-07 14:34:47.485 +0200
OS Version:      Mac OS X 10.6.8 (10K549)
Report Version:  6

Exception Type:  EXC_BREAKPOINT (SIGTRAP)
Exception Codes: 0x0000000000000002, 0x0000000000000000
Crashed Thread:  0

Dyld Error Message:
  Symbol not found: _OBJC_CLASS_$_NSTableCellView
  Referenced from: /Applications/Quicksilver.app/Contents/MacOS/../Frameworks/QSCore.framework/Versions/A/QSCore
  Expected in: /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
 in /Applications/Quicksilver.app/Contents/MacOS/../Frameworks/QSCore.framework/Versions/A/QSCore

(...plus the "Binary Images:" stuff...)

A few messages on the console that seem to be related:

7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Headers': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `QSCore': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Resources': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Current': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Headers': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `QSEffects': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Resources': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Current': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Headers': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `QSFoundation': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Resources': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Current': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Headers': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `QSInterface': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Resources': 93
7/9/13 14:34:46     kernel  CoreServicesUIAg[799] Unable to clear quarantine `Current': 93
7/9/13 14:34:47     [0x0-0x6b06b].com.blacktree.Quicksilver[798]    dyld: Symbol not found: _OBJC_CLASS_$_NSTableCellView
7/9/13 14:34:47     [0x0-0x6b06b].com.blacktree.Quicksilver[798]      Referenced from: /Applications/Quicksilver.app/Contents/MacOS/../Frameworks/QSCore.framework/Versions/A/QSCore
7/9/13 14:34:47     [0x0-0x6b06b].com.blacktree.Quicksilver[798]      Expected in: /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
7/9/13 14:34:47     [0x0-0x6b06b].com.blacktree.Quicksilver[798]     in /Applications/Quicksilver.app/Contents/MacOS/../Frameworks/QSCore.framework/Versions/A/QSCore
7/9/13 14:35:20     com.apple.launchd.peruser.501[101]  ([0x0-0x6b06b].com.blacktree.Quicksilver[798]) Job appears to have crashed: Trace/BPT trap
7/9/13 14:35:21     ReportCrash[801]    Saved crash report for Quicksilver[798] version 1.1.0:minVersion (4004) to /Users/HenningJ/Library/Logs/DiagnosticReports/Quicksilver_2013-09-07-143521_Henning-Jungkurths-MacBook.crash

@tiennou
Copy link
Member

@tiennou tiennou commented Sep 7, 2013

Here's my reference : http://www.sealiesoftware.com/blog/archive/2009/09/09/objc_explain_Weak-import_classes.html

Weak-linked classes are only available from 10.7+, so there's not amount of fiddling that can be done to make our QS version work (unless you drop the nice plugin update window, which I think the NSTableCellView is responsible for...

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 7, 2013

Or we build a helper app that loads QS if it's deemed compatible or displays an error message.

See my post on the qsdev forums for more info, but here's the link to the article I reference

http://blog.bignerdranch.com/13-requiring-a-minimum-version-of-os-x-for-your-application/

On 7 Medi 2013, at 23:29, Etienne Samson notifications@github.com wrote:

Here's my reference : http://www.sealiesoftware.com/blog/archive/2009/09/09/objc_explain_Weak-import_classes.html

Weak-linked classes are only available from 10.7+, so there's not amount of fiddling that can be done to make our QS version work (unless you drop the nice plugin update window, which I think the NSTableCellView is responsible for...


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 8, 2013

Right, back at a real computer now…

So our worst fears were realised - Henning still gets the crash no matter where the popup is placed in the code.

I've looked more into the approach of http://blog.bignerdranch.com/13-requiring-a-minimum-version-of-os-x-for-your-application/, and whilst the download's now dead, there is some code on GitHub:
https://github.com/jessegrosjean/systemversioncheck

It's old, but it still works fine. Basically we'd just need to edit the release build to rename the Quicksilver executable to Quicksilver.real then place the SystemVersionCheck executable in its place.

Thoughts?

On 8 Medi 2013, at 07:17, Patrick Robertson robertson.patrick@gmail.com wrote:

Or we build a helper app that loads QS if it's deemed compatible or displays an error message.

See my post on the qsdev forums for more info, but here's the link to the article I reference

http://blog.bignerdranch.com/13-requiring-a-minimum-version-of-os-x-for-your-application/

On 7 Medi 2013, at 23:29, Etienne Samson notifications@github.com wrote:

Here's my reference : http://www.sealiesoftware.com/blog/archive/2009/09/09/objc_explain_Weak-import_classes.html

Weak-linked classes are only available from 10.7+, so there's not amount of fiddling that can be done to make our QS version work (unless you drop the nice plugin update window, which I think the NSTableCellView is responsible for...


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 8, 2013

P.S. it's what Transmission and Adium both use by the looks of things:

https://trac.transmissionbt.com/attachment/ticket/346/SystemVersionCheck.patch

On 8 Medi 2013, at 07:17, Patrick Robertson robertson.patrick@gmail.com wrote:

Or we build a helper app that loads QS if it's deemed compatible or displays an error message.

See my post on the qsdev forums for more info, but here's the link to the article I reference

http://blog.bignerdranch.com/13-requiring-a-minimum-version-of-os-x-for-your-application/

On 7 Medi 2013, at 23:29, Etienne Samson notifications@github.com wrote:

Here's my reference : http://www.sealiesoftware.com/blog/archive/2009/09/09/objc_explain_Weak-import_classes.html

Weak-linked classes are only available from 10.7+, so there's not amount of fiddling that can be done to make our QS version work (unless you drop the nice plugin update window, which I think the NSTableCellView is responsible for...


Reply to this email directly or view it on GitHub.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Sep 8, 2013

Yay, go built me a test app and I'll...well...test it. :-)

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 8, 2013

You don't think it's sufficient to make sure the auto-update system doesn't advertise the new version to 10.6, and also clearly identify compatible OSes on the manual download page?

Just asking, since it seems like a good amount of work, and bit of a messy solution for a very small number of people. I also worry that always launching via a helper app would prevent launchDate from being set, as discussed under http://projects.skurfer.com/QuicksilverPlug-inReference.mdown#indexisvalidfromdate

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 10, 2013

I don't think the launchDate business would be affected, but it might.
I had a quick chat with Etienne about it on IRC and he also pointed out that we'd need to make sure newly build plugins were correctly linked against the Quicksilver.real file (as opposed to the dummy file), which could be a problem.

For the time being hopefully what we have is enough… I can't be bothered to go through all that right now ;-)

On 9 Medi 2013, at 04:37, Rob McBroom notifications@github.com wrote:

You don't think it's sufficient to make sure the auto-update system doesn't advertise the new version to 10.6, and also clearly identify compatible OSes on the manual download page?

Just asking, since it seems like a good amount of work, and bit of a messy solution for a very small number of people. I also worry that always launching via a helper app would prevent launchDate from being set, as discussed under http://projects.skurfer.com/QuicksilverPlug-inReference.mdown#indexisvalidfromdate


Reply to this email directly or view it on GitHub.

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.

None yet

4 participants