Fixing version comparison & adding support for build comparisons#69
Fixing version comparison & adding support for build comparisons#69pudquick wants to merge 1 commit intogoogle:masterfrom
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
CLAs look good, thanks! |
| } else { | ||
| NSLog(@"Checking: OS build not found in expected builds [ %@ ]", expectedBuildsStr); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic, while correct, is a bit more difficult to read as it's quite repetitive. Could we flip it around?
if ([expectedVersionArray isEqualToArray:systemVersionArray]) {
NSLog(@"Checking: running OS is equal to %@, checking build version", expectedVersion);
if (!expectedBuilds.count) {
NSLog(@"Exiting: No preferred build, so considered equal");
[NSApp terminate:nil];
}
if ([expectedBuilds containsObject:systemBuild]) {
NSLog(@"Exiting: OS build is one of the expected builds %@", expectedBuilds);
[NSApp terminate:nil];
}
NSLog(@"Checking: OS build not found in expected builds %@", expectedBuilds);
} else if ([systemVersionArray[0] intValue] < [expectedVersionArray[0] intValue] ||
[systemVersionArray[1] intValue] < [expectedVersionArray[1] intValue] ||
[systemVersionArray[2] intValue] < [expectedVersionArray[2] intValue]) {
NSLog(@"Checking: OS build is lower than required");
} else {
NSLog(@"Exiting: OS build is greater than required");
[NSapp terminate:nil];
}| ([expectedVersionArray[1] intValue] == [systemVersionArray[1] intValue]) && | ||
| ([expectedVersionArray[2] intValue] == [systemVersionArray[2] intValue])) { | ||
| NSLog(@"Checking: OS is equal to %@, checking build version", expectedVersion); | ||
| if (expectedBuilds == nil) { |
There was a problem hiding this comment.
Use expectedBuilds.count == 0 or !expectedBuilds.count to catch both nil and an empty array
| if ([expectedBuilds containsObject:systemBuild]) { | ||
| NSLog(@"Exiting: OS build is one of the expected builds [ %@ ]", expectedBuildsStr); | ||
| [NSApp terminate:nil]; | ||
| } else { |
There was a problem hiding this comment.
As the last line of the first condition exits, there's no need for the else, might as well remove it and reduce the indentation.
| - (void)applicationDidFinishLaunching:(NSNotification *)aNotification { | ||
| NSString *expectedVersion = NSLocalizedString(@"expectedVersion", @""); | ||
| NSString *expectedBuildsStr = NSLocalizedString(@"expectedBuilds", @""); | ||
| NSArray *expectedBuilds; |
There was a problem hiding this comment.
nit: For large expectedBuilds builds list, a set theoretically has faster lookups.
NSString *buildsKey = @"expectedBuilds";
NSString *builds = NSLocalizedString(buildsKey, @"");
NSSet *expectedBuilds = [NSSet setWithArray:[builds componentsSeparatedByString:@","]];
if (!expectedBuilds.count || [expectedBuilds containsObject:buildsKey]) expectedBuilds = nil;
This is both a bug fix and a new feature.
First the bug fix - here's an example situation:
This logic: https://github.com/google/macops/blob/master/deprecation_notifier/DeprecationNotifier/DNAppDelegate.m#L60-L63
10 <= 10: true
14 <= 15: true
3 <= 1: false
The comparison will fail when it shouldn't.
This PR includes better comparison logic.
It additionally adds support for a new optional Localizable string value: expectedBuilds
The value of this string should be a comma delimited string (no spaces) of allowed builds.
If the OS is detected to be exactly the expectedVersion, then:
Use case: for environments that are supporting a major OS version which has now gone into Security Updates, Deprecation Notifier is unable to tell a "fully patched" OS version without looking at the build information.