Skip to content

Fix the way to check OS version#662

Merged
splhack merged 2 commits intomacvim-dev:masterfrom
ichizok:fix/version-check
Apr 19, 2018
Merged

Fix the way to check OS version#662
splhack merged 2 commits intomacvim-dev:masterfrom
ichizok:fix/version-check

Conversation

@ichizok
Copy link
Copy Markdown
Member

@ichizok ichizok commented Mar 28, 2018

Improved #659

When MACOSX_DEPLOYMENT_TARGET is prior than 10.10, we can check OS version by reading ProductVersion from /System/Library/CoreServices/SystemVersion.plist.

@ichizok ichizok force-pushed the fix/version-check branch from f6c6abe to 97e6403 Compare April 3, 2018 04:51
Comment thread src/MacVim/gui_macvim.m Outdated
[[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:version];
}
#endif
check_system_version();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ([NSProcessInfo instancesRespondToSelector:@selector(isOperatingSystemAtLeastVersion:)]) {
  NSOperatingSystemVersion version = {10, 13, 0};
  is_macos_high_sierra_or_later = [[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:version];	
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I updated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant we don't need check_system_version method, specially the SystemVersion.plist part since isOperatingSystemAtLeastVersion: is available on 10.10 or later.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When build MacVim with env MACOSX_DEPLOYMENT_TARGET < 10.10 (.travis.yml has set =10.8) isOperatingSystemAtLeastVersion: isn't available, so cannot build.
Therefore SystemVersion.plist part is needed in order to use MacVim built with env MACOSX_DEPLOYMENT_TARGET < 10.10 on 10.13.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to support that situation. Using SDK 10.9 or earlier and run the binary on 10.13 or later. It's overkill. We do support the latest Xcode + the latest SDK, and the binary could run on 10.8. Probably people can use SDK 10.8 for building MacVim for 10.8, but not for 10.13.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the minimal code would look like

}

#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_10
    if ([[NSProcessInfo processInfo]
            respondsToSelector:@selector(isOperatingSystemAtLeastVersion:)])
    {
        NSOperatingSystemVersion version = {10, 13, 0};

        is_macos_high_sierra_or_later =
            [[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:version];
    }
#endif

    return OK;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On travis build (MACOSX_DEPLOYMENT_TARGET=10.8), MAC_OS_X_VERSION_MAX_ALLOWED is MAC_OS_X_VERSION_10_8 so that code block is dropped.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current master .travis.yml uses xcode9.2, so MAC_OS_X_VERSION_MAX_ALLOWED would be 10.12 or 10.13, and MAC_OS_X_VERSION_MIN_REQUIRED is 10.8 as MACOSX_DEPLOYMENT_TARGET in .travis.yml.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, MAC_OS_X_VERSION_MAX_ALLOWED... I saw wrong. OK, I understand.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated.

@ichizok ichizok force-pushed the fix/version-check branch from 97e6403 to 6a63a82 Compare April 13, 2018 02:38
@ichizok ichizok force-pushed the fix/version-check branch from 6a63a82 to dd4c394 Compare April 19, 2018 06:50
@splhack splhack merged commit 696a33e into macvim-dev:master Apr 19, 2018
@ichizok ichizok deleted the fix/version-check branch April 19, 2018 07:10
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.

2 participants