Skip to content

Change several deprecated symbols in _macosx.m #18128

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

Merged
merged 2 commits into from
Aug 24, 2020

Conversation

jkseppan
Copy link
Member

The compiler warns that some of these have been deprecated in
MacOS 10.12, some in 10.14.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

@tacaswell tacaswell added this to the v3.4.0 milestone Jul 30, 2020
@tacaswell
Copy link
Member

How far back to the non-deprecated symbols exist? With 3.3 (see #18066 and #17956 ) we accidentally put a floor on the version of OSX to support.

What is below is a brain dump.

I think that OSX SDK version should fall under the "system and C-dependencies" category of https://matplotlib.org/devel/min_dep_policy.html so we should should support "as old as practical". We should define what "practical" means in this case (as per #18066 ).

Given discussion in macports/macports-ports@b07e0eb it seems at least some people are advocating for a pattern of building on new OSX but being able to run on older OSX (which fits with how we do the wheels), but leaves people who a) have very old macs b) want to build from source them selves in a bind.

The built Python on python.org reportedly supports back to 10.9 (I suspect because they are not using much of the SDK surface particularly around GUI work the way we do), macports also tries to support back very far (but that is their jam).

@jkseppan
Copy link
Member Author

Right, the new symbols probably don't exist in 10.11.

It looks like Travis can run builds in OSX 10.11 so once the minimum version is decided, perhaps we should test the build in that version: https://docs.travis-ci.com/user/reference/osx/#macos-version

@jkseppan
Copy link
Member Author

I think that second commit is the way to stay compatible, but I can't actually properly test myself. I recently had to upgrade my remaining 10.13 system, because various pieces of software were dropping support.

@dopplershift
Copy link
Contributor

Unfortunately, due to @available I don't think we'll build on anything earlier than the xcode9 image, which is macOS 10.12. Might be worth giving that a shot here and seeing where we are.

👍 to the changes.

@jkseppan jkseppan force-pushed the macos-deprecations branch from 16309ca to c97b09e Compare July 31, 2020 05:37
@jkseppan jkseppan marked this pull request as draft July 31, 2020 07:00
@jkseppan
Copy link
Member Author

Converting to draft until I can figure out the CI build

@tacaswell
Copy link
Member

Unfortunately, due to @available I don't think we'll build on anything earlier than the xcode9 image, which is macOS 10.12. Might be worth giving that a shot here and seeing where we are.

Given that macports is happy (for now) to patch us and are considering moving to build on newer systems to target older systems this seems like a good place to draw the line.

The compiler warns that some of these have been deprecated in
MacOS 10.12, some in 10.14.
@jkseppan jkseppan force-pushed the macos-deprecations branch from c97b09e to ae316b4 Compare August 2, 2020 17:40
@jkseppan jkseppan marked this pull request as ready for review August 2, 2020 17:42
@tacaswell tacaswell merged commit abeb3cb into matplotlib:master Aug 24, 2020
@tacaswell
Copy link
Member

Thanks @jkseppan !

@QuLogic
Copy link
Member

QuLogic commented Aug 24, 2020

This does not build wheels when targeting 10.9: https://github.com/matplotlib/matplotlib/actions/runs/221342241

@dopplershift
Copy link
Contributor

I thought we weren't ourselves supporting building on 10.9?

@QuLogic
Copy link
Member

QuLogic commented Aug 24, 2020

Hmm, well the MacPython wheels still specify 10.9, but maybe we determined that that had no effect? And now cibuildwheel is correctly passing that along?

@tacaswell
Copy link
Member

I'm very confused by this though. The PR is doing

#define CGContext                            graphicsPort

but the error we get is:

  src/_macosx.m:1661:59: error: 'CGContext' is only available on macOS 10.10 or newer [-Werror,-Wunguarded-availability]
1196
      CGContextRef cr = [[NSGraphicsContext currentContext] CGContext];
1197
                                                            ^~~~~~~~~~
1198
  /Applications/Xcode_11.6.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSGraphicsContext.h:81:35: note: 'CGContext' has been marked as being introduced in macOS 10.10 here, but the deployment target is macOS 10.9.0
1199
  @property (readonly) CGContextRef CGContext NS_RETURNS_INNER_POINTER API_AVAILABLE(macos(10.10));
1200

which makes me worry I don't understand what #define does in objective-c....

@tacaswell
Copy link
Member

or at least the order in which the version checking vs marco expansion happens...

@@ -18,6 +18,40 @@
#define COMPILING_FOR_10_10
#endif

#if __MAC_OS_X_VERSION_MAX_ALLOWED < 101200
Copy link
Member

Choose a reason for hiding this comment

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

Should you not be checking for minimum required, like above?

@QuLogic
Copy link
Member

QuLogic commented Aug 29, 2020

So what do we want to do about this? Target a newer macOS version? It also breaks building on the shared Mac mini.

@jkseppan
Copy link
Member Author

I think we should make a decision about the minimum version of macOS that we support (#18066). Based on this comment by @tacaswell I would suggest making that 10.12:

Unfortunately, due to @available I don't think we'll build on anything earlier than the xcode9 image, which is macOS 10.12. Might be worth giving that a shot here and seeing where we are.

Given that macports is happy (for now) to patch us and are considering moving to build on newer systems to target older systems this seems like a good place to draw the line.

Whichever way that's decided, we can build wheels targeting the supported version and up, and fix the build as needed.

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

Successfully merging this pull request may close these issues.

4 participants