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

Fix version comparisons for 10.10 #1939

Merged
merged 11 commits into from Oct 16, 2014
Merged

Fix version comparisons for 10.10 #1939

merged 11 commits into from Oct 16, 2014

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 1, 2014

A simple change really.

But then, I thought I'd be clever and write unit tests. Turns out they're a mess in XC6. I'm half way to getting them right...

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 1, 2014

Some of these commits may be of interest to @HenningJ
It seems that Apple no longer wants tests to be run 'directly' (instead, they should be run by using the 'test' action for a given scheme)
As such, the 'Run All Tests' target you made doesn't work (it just builds the test targets and doesn't run them). There's now also no point in having schemes specifically for the test targets.

I've removed these, and added a script that runs all the tests. Namely:

#!/usr/bin/env sh

xcodebuild test -scheme "QuickStep Core"
xcodebuild test -scheme "QuickStep Foundation"
xcodebuild test -scheme "Core Support"
xcodebuild test -scheme "Quicksilver"

This seems to make more sense to me, and should hopefully be easier to automate. I'll now have a play with trying to run tests without a window session. If there is a way I could get access to Jenkins that would be useful.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 1, 2014

Aside: It may be worth running ./Tools/qstest from qsrelease, so that just before it's built and released, the tests are run

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 1, 2014

It would be good to include the tests in the release process, but we need to fix it up a bit first.

As for the version comparison, the problem was in -[QSPlugIn meetsRequirements:]. I’ll look at what you changed, because that was probably broken, too.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 1, 2014

As for the version comparison, the problem was in -[QSPlugIn meetsRequirements:]

Oh, ok - so not my problem. I may well have not fixed anything. I basically assumed that we’d now be checking if “1090 > 1010” (10.9 > 10.10) and I thought that was where the problem was. I may well have been completely wrong. But my code should be more explicit anyway

On 1 Hyd 2014, at 14:05, Rob McBroom notifications@github.com wrote:

It would be good to include the tests in the release process, but we need to fix it up a bit first.

As for the version comparison, the problem was in -[QSPlugIn meetsRequirements:]. I’ll look at what you changed, because that was probably broken, too.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 4, 2014

OK, that should fix it. I can’t tell you how much I hated writing so much code to do something so simple. 😃

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 4, 2014

Whoops. Kinda made that method single-purpose the first time. Fixed, squashed, and force-pushed.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 5, 2014

I was going to suggest that the dottedVersionCompare stuff should really stay in NSApplication_BLTRExtensions along with all the other version-checking code, so as to keep it in one place, but now I see you're using an NSComparison maybe it wouldn't work.

I suppose you could use something like:
[NSApplication compareOSXVersionTo:osUnsupported] which might be tidier.

A lot of the NSString code you've written is kinda a duplication of the code that I'd written for this pull, so it would be best to keep it together. Perhaps it's my new changes to isLeopard, isSnowLeopard etc. that should use your new NSString methods.

Anyway - food for thought :)

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 6, 2014

I suppose you could use something like:
[NSApplication compareOSXVersionTo:osUnsupported] which might be tidier.

Yeah, but it’s meant to be a general purpose comparison tool. Not necessarily just the OS version.

Perhaps it's my new changes to isLeopard, isSnowLeopard etc. that should use your new NSString methods.

Looking at it again, maybe we should do that. Your way is faster (because it can be - we know what we’re getting), but if Apple ever did release an OS 11, those methods would start failing as currently written.

I can make the change if you agree.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 6, 2014

I realized that all of those methods would end up looking like this

return ([[NSApplication macOSXFullVersion] dottedVersionCompare:@"10.5.0"] == NSOrderedSame || [[NSApplication macOSXFullVersion] dottedVersionCompare:@"10.5.0"] == NSOrderedAscending);

which is ugly and expensive. So I think we should stick with simple integer comparisons. I just tweaked them a bit.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 8, 2014

This is it, before 1.2.0 final. (Probably. 😬) Any concerns?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 8, 2014

are you happy with the unit test changes? if that's fine, decide whatever you want RE my comment just now, then merge away!

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 8, 2014

I looked at the unit test stuff when this was new, but should look again.

My main problem is that I didn’t add any tests for my new method. 😢 I’ll try to do that ASAP before merging, but if Yosemite gets released first, I’ll go ahead with this.

@pjrobertson pjrobertson mentioned this pull request Oct 9, 2014
skurfer added 3 commits Oct 10, 2014
- include license
- removed unnecessary import statements from SUStandardVersionComparator.m
@skurfer
Copy link
Member

@skurfer skurfer commented Oct 10, 2014

OK, try that. There was a never-ending chain of dependencies with Sparkle’s #imports, so I stopped and checked and it turned out none of the ones in SUStandardVersionComparator.m were necessary, so I removed them.

Tested the change by forcing the version to 10.10.0, 10.10, 10.8, and 10.8.6. Also tested under an actual Yosemite VM.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 16, 2014

As promised, if Yosemite comes out before this is merged, I’ll merge it and release (probably 6-8 hours from now). If you have any objections, now’s the time…

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 16, 2014

Sorry, I'm swamped.

Code looks fine, and nice to see the release in there.
I haven't had time to test it... but go ahead :)

pjrobertson added a commit that referenced this issue Oct 16, 2014
Fix version comparisons for 10.10
@pjrobertson pjrobertson merged commit acfced7 into master Oct 16, 2014
0 of 2 checks passed
@pjrobertson pjrobertson deleted the i1938 branch Oct 16, 2014
skurfer added a commit that referenced this issue Oct 17, 2014
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

3 participants