8000 BUG: fix compilation of 3rd party modules with Py_LIMITED_API enabled by mshabunin · Pull Request #13725 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: fix compilation of 3rd party modules with Py_LIMITED_API enabled #13725

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 13, 2019
Merged

BUG: fix compilation of 3rd party modules with Py_LIMITED_API enabled #13725

merged 1 commit into from
Jun 13, 2019

Conversation

mshabunin
Copy link
Contributor

There are no macros PyTuple_GET_SIZE and PyTuple_GET_ITEM available when compiling with enabled Py_LIMITED_API.

Breaking change has been made in PR #10860. Before the change these macros had been used only in NPY_TITLE_KEY macro and had not been compiled not used. After the change they have been moved to a function and now are always visible to compiler. Other projects using numpy C-API can not be compiled with Py_LIMITED_API macro enabled because of this problem.

@mhvk
Copy link
Contributor
mhvk commented Jun 6, 2019

@mshabunin - agree we should fix this, but my feeling is that either we just don't use the macros here (this doesn't seem like a critical path, performance-wise) or that we define them if they are not present in, e.g., npy_3kcompat.h. cc @eric-wieser, who I think has thought about this a bit more (than me, at least)

@mattip
Copy link
Member
mattip commented Jun 6, 2019

I wonder how we could test the limited API with a CI task. It would need to somehow call all the NumPy C-API functions in order to find the linking errors.

@mattip mattip added this to the 1.17.0 release milestone Jun 6, 2019
@mattip mattip added the 09 - Backport-Candidate PRs tagged should be backported label Jun 6, 2019
@mhvk
Copy link
Contributor
mhvk commented Jun 6, 2019

@mshabunin - given the comment by @mattip, just to be sure I understand the issue here: it is about ensuring we do only use limited API in the header files, correct? You don't really care about what we do inside our own source code. If so, would a possible regression test case be a tiny .c programme that defines the limited API and includes the standard numpy headers?

@mshabunin
Copy link
Contributor Author

@mhvk , yes it is about header files only. Sure, I'll try to make a test.

@eric-wieser
Copy link
Member

Should we just move NPY_TITLE_KEY_check into the exported C api?

@mhvk
Copy link
Contributor
mhvk commented Jun 7, 2019

Should we just move NPY_TITLE_KEY_check into the exported C api?

Makes sense to me - not great to have code in the headers anyway, and it is only used in the #define NPY_TITLE_KEYjust below (which itself is used in not that many places).

@mshabunin
Copy link
Contributor Author

So, should I move it to sources? Is there any "compile an app with numpy" test exist already?

tylerjereddy
tylerjereddy reviewed Jun 11, 2019
View reviewed changes
Copy link
Contributor
@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

Using the latest NumPy master branch I can reproduce the issue with a 2-line C test file:

#define Py_LIMITED_API
#include "numpy/ndarrayobject.h"

Building will error if the right flags are chosen:
gcc -c test.c $(python3.6-config --includes) -I/python3.6/site-packages/numpy/core/include -Werror=implicit-function-declaration

Removing the first line allows it to build.

Not sure if we'd want to do something like that & build inclusion of headers up from there, but it might spur 8000 suggestions from the NumPy C API gurus in any case.

@mhvk
Copy link
Contributor
mhvk commented Jun 12, 2019

Since 1.17 is about to be branched, perhaps we should separate the fix from the test (I like @tylerjereddy's super-simple example very much, but it may need some thought about how to actually do the test).

For the fix, my suggestion would be to just not use anything from the limited API in our headers. So, for now, just replace the code rather than special-case it. Eventually, we can move the whole thing to a C function, where as @eric-wieser argues correctly, it really belongs. But might as well make that a separate issue.

@eric-wieser
Copy link
Member

But might as well make that a separate issue.

Yep, seems fine to me.

@mattip
Copy link
Member
mattip commented Jun 13, 2019

just replace the code rather than special-case it

It seems there is consensus that the fix should be non-conditional.

@mshabunin could you make this change?

@mshabunin
BUG: fix compilation of 3rdparty modules with Py_LIMITED_API enabled
b36f02b 
There are no macros PyTuple_GET_SIZE and PyTuple_GET_ITEM available when compiling with enabled Py_LIMITED_API.
@mshabunin mshabunin force-pushed the fix-limited-api-build branch from b1e3c09 to b36f02b Compare June 13, 2019 08:21
@mshabunin
Copy link
Contributor Author

@mattip , done.

@mattip
Copy link
Member
mattip commented Jun 13, 2019

Perhaps leave the test for another PR or, since tests in CI are difficult to maintain and debug, try something like below

def test_headers():
    from numpy.distutils import ccompiler
    gcc = ccompiler.new_compiler()
    with open(tmpfilename, 'wt') as fid:
        fid.write(textwrap.dedent('''
            #define Py_LIMITED_API
            #include ..."
            int main(int argc, const char * argv[]) {
                return 0;
           };'''))
    gcc.compile(tmpfilename, include_dirs = numpy.get_include())

@mshabunin mshabunin force-pushed the fix-limited-api-build branch from c65339e to b36f02b Compare June 13, 2019 10:59
@mshabunin
Copy link
Contributor Author

@mattip , thank you! I'll make a new PR with the test.

@mattip mattip merged commit e5425cd into numpy:master Jun 13, 2019
@mattip mattip mentioned this pull request Jun 14, 2019
Closed
@charris charris modified the milestones: 1.17.0 release, 1.16.5 release Jul 14, 2019
@charris charris changed the title BUG: fix compilation of 3rdparty modules with Py_LIMITED_API enabled BUG: fix compilation of 3rd party modules with Py_LIMITED_API enabled Jul 18, 2019
@charris charris mentioned this pull request Jul 18, 2019
Merged
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jul 18, 2019
@charris charris removed this from the 1.16.5 release milestone Jul 18, 2019
@mshabunin mshabunin deleted the fix-limited-api-build branch July 14, 2020 17:17
@alalek alalek mentioned this pull request Jan 16, 2022
Closed
@aforren1 aforren1 mentioned this pull request May 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerjereddy tylerjereddy tylerjereddy left review comments

Assignees
No one assigned
Labels
00 - Bug 30 - API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mshabunin @mhvk @mattip @eric-wieser @tylerjereddy @charris
0