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

More fixes to Numpy so that the binary is accepted by the App Store #473

Merged
merged 1 commit into from
May 5, 2020

Conversation

lerela
Copy link
Contributor

@lerela lerela commented May 4, 2020

See #409 for context

AndreMiras
AndreMiras previously approved these changes May 4, 2020
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Great findings, thanks for the pull request!
This is actually hard to understand without the full context. It feels like "why are we ever just renaming functions".
Digging around I understood, the Apple store automatic app checks get mislead as if we were trying to reference to Apple non public symbols while we actually reference our own private symbols.
As this is a rather tricky one and could be overlooked and reverted by mistake in the future I would give special care by either commenting the patch file itself comprehensively or having a more detailed commit message, at least the reference to the bug #409
The problem is with or without this patch numpy would comply properly hence we may break what you just fixed (Apple store submission) without realising.
Last thing, have you reported the bug to numpy upstream (to save other people time)? Because applying the fix is actually simple, but digging it probably was painful

return []
return [all_sources[0]]

+ def get_lapack_lite_sources_ios(ext, build_dir):
+ return []
+ return [all_sources[0]]
Copy link
Member

Choose a reason for hiding this comment

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

isn't this ending up behaving the same way as originally before being patched at all?
https://github.com/numpy/numpy/blob/v1.16.4/numpy/linalg/setup.py#L39

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, fixed

@lerela
Copy link
Contributor Author

lerela commented May 5, 2020

It seems to me that Numpy is referencing private Apple symbols as the file umath_linalg.c.src only wraps BLAS and LAPACK routines. Since lapack_info is not None in numpy/linalg/setup.py, Numpy's Lapack emulation is not compiled, yet umath_linalg.c.src expects those symbols.
They are found in some Apple framework (most likely Accelerate) during linking but as they are not public, Apple will reject the application later on.

An explanation could be that these symbols are only private on iOS, which would explain why Numpy is working fine on MacOS. I will report these findings to them, thanks for the suggestion!

@lerela lerela force-pushed the 409_numpy_issues branch 2 times, most recently from 14376ba to 79e0d19 Compare May 5, 2020 07:49
@AndreMiras
Copy link
Member

OK that makes even more sense now thank you.
Thanks for editing/squashing the commit with detailed explanation. I'm ready for a merge once we have solved the conflicts.
We recently moved the source code (including recipes/) to the kivy_ios/ directory. Could you please rebase/update the PR so I can merge it?
I'm on Discord if you need some support and I can also do it for you if it's too painful

Explanation: Apple forbids some symbols that Numpy is linked against (ccopy, dcopy, scopy, zcopy and xerbla). Compilation and tests work fine but the App Store complains and prevents uploading such builds.
This patch aliases the culprits to the public, allowed method names, allowing the linking to proceed as before and the resulting binary to be accepted by the App Store.
@lerela
Copy link
Contributor Author

lerela commented May 5, 2020

Done, however I've not rebuilt Kivy/Numpy with the new structure to make sure it'd work!

@AndreMiras
Copy link
Member

Thanks! And the CI built it green for you it seems :)
We make a quick check and merge

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Looking good, thank you for addressing the comments

@AndreMiras AndreMiras merged commit 9f8e079 into kivy:master May 5, 2020
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