GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-28411: Fix redundant declaration of _PyImport_AddModuleObject by jeremycline · Pull Request #7992 · python/cpython

Conversation

jeremycline commented Jun 28, 2018 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Commit 3f9eee6 ("bpo-28411: Support other mappings in
PyInterpreterState.modules.") introduced a new warning when building a C
extension with GCC and Python 3.7:

In file included from /usr/include/python3.7m/Python.h:126,
                 from /home/me/linux/packaging/tools/perf/util/python.c:2:
/usr/include/python3.7m/import.h:58:24: error: redundant redeclaration of
    ‘_PyImport_AddModuleObject’ [-Werror=redundant-decls]
 PyAPI_FUNC(PyObject *) _PyImport_AddModuleObject(PyObject *, PyObject *);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/python3.7m/import.h:47:24: note: previous declaration of
    ‘_PyImport_AddModuleObject’ was here
 PyAPI_FUNC(PyObject *) _PyImport_AddModuleObject(PyObject *name,
                        ^~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
error: command 'gcc' failed with exit status 1

Both _PyImport_AddModuleObject declarations are within #ifndef Py_LIMITED_API
conditional block so drop one of them.

Signed-off-by: Jeremy Cline [email protected]

https://bugs.python.org/issue28411

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix redundant declaration of _PyImport_AddModuleObject

5c6e745

Commit 3f9eee6 ("bpo-28411: Support other mappings in
PyInterpreterState.modules.") introduced a new warning when building a C
extension with GCC and Python 3.7:

    In file included from /usr/include/python3.7m/Python.h:126,
                     from /home/me/linux/packaging/tools/perf/util/python.c:2:
    /usr/include/python3.7m/import.h:58:24: error: redundant redeclaration of
        ‘_PyImport_AddModuleObject’ [-Werror=redundant-decls]
     PyAPI_FUNC(PyObject *) _PyImport_AddModuleObject(PyObject *, PyObject *);
                            ^~~~~~~~~~~~~~~~~~~~~~~~~
    /usr/include/python3.7m/import.h:47:24: note: previous declaration of
        ‘_PyImport_AddModuleObject’ was here
     PyAPI_FUNC(PyObject *) _PyImport_AddModuleObject(PyObject *name,
                            ^~~~~~~~~~~~~~~~~~~~~~~~~
    cc1: all warnings being treated as errors
    error: command 'gcc' failed with exit status 1

Both _PyImport_AddModuleObject declarations are within #ifndef Py_LIMITED_API
conditional block so drop one of them.

Signed-off-by: Jeremy Cline <[email protected]>

the-knights-who-say-ni commented Jun 28, 2018

Copy link

Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

Thanks again for your contribution, we look forward to reviewing it!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

the-knights-who-say-ni added the CLA not signed label

Jun 28, 2018

bedevere-bot added the awaiting review label

Jun 28, 2018

serhiy-storchaka requested a review from ericsnowcurrently

June 29, 2018 10:03

ericsnowcurrently approved these changes

Jun 29, 2018

View reviewed changes

ericsnowcurrently left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yep, that duplicate slipped back in as I rolled back out some of the changes from GH-1638 (reverted in GH-3565). GH-3575 added back the definition from the original change. Then GH-3593 added the duplicate.

I expect I intended on moving the definition restored in GH-3575 and accidentally left the old one in. :) So I'm fine with the PR as-is.

FYI, the PR is missing the issue number in the title (BPO-28411). It should probably also have a "skip news" label, since this is a small, otherwise transparent fix against GH-3593. We can do both, but will give you the chance first. :)

Finally, the bot noted you have not signed the CLA yet. While we are fine merging this PR without the CLA (it's a simple and obvious fix), we'd still recommend getting the CLA done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting review labels

Jun 29, 2018

jeremycline changed the title ~~Fix redundant declaration of _PyImport_AddModuleObject~~ bpo-28411: Fix redundant declaration of _PyImport_AddModuleObject

Jun 29, 2018

jeremycline commented Jun 29, 2018

Copy link

Copy Markdown

Contributor Author

FYI, the PR is missing the issue number in the title (BPO-28411). It should probably also have a "skip news" label, since this is a small, otherwise transparent fix against GH-3593. We can do both, but will give you the chance first. :)

Cool, I've fixed the title. It seemed too trivial to file a bug for and I wasn't sure if associating it with the original bug was normal protocol. I don't think I can add the label (or I'm missing something in the UI).

Finally, the bot noted you have not signed the CLA yet. While we are fine merging this PR without the CLA (it's a simple and obvious fix), we'd still recommend getting the CLA done.

I made a bugs.python.org account and submitted the CLA yesterday. It doesn't look like it's been confirmed yet, but it's not really been a full business day either 😄

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently added the skip news label

Jun 29, 2018

ericsnowcurrently merged commit f874bd1 into python:master

Jun 29, 2018

bedevere-bot removed the awaiting merge label

Jun 29, 2018

ericsnowcurrently added the needs backport to 3.7 label

Jun 29, 2018

miss-islington commented Jun 29, 2018

Copy link

Copy Markdown

Contributor

Thanks @jeremycline for the PR, and @ericsnowcurrently for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.7 label

Jun 29, 2018

bedevere-bot commented Jun 29, 2018

Copy link

Copy Markdown

GH-8017 is a backport of this pull request to the 3.7 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jun 29, 2018


          Fix redundant declaration of _PyImport_AddModuleObject (pythonGH-7992)

52291ee

(cherry picked from commit f874bd1)

Co-authored-by: Jeremy Cline <[email protected]>

miss-islington added a commit that referenced this pull request

Jun 29, 2018


          Fix redundant declaration of _PyImport_AddModuleObject (GH-7992)

c919252

(cherry picked from commit f874bd1)

Co-authored-by: Jeremy Cline <[email protected]>

jeremycline deleted the fix-duplicate-decl branch

June 30, 2018 05:29

Mariatta removed the CLA not signed label

Aug 30, 2018

the-knights-who-say-ni added the CLA signed label

Aug 30, 2018

ericsnowcurrently mentioned this pull request

Apr 10, 2022

[subinterpreters] Eliminate PyInterpreterState.modules. #72597

Open

Reviewers

ericsnowcurrently

ericsnowcurrently approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

jeremycline commented Jun 28, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jun 28, 2018

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

jeremycline commented Jun 29, 2018

Uh oh!

miss-islington commented Jun 29, 2018

Uh oh!

bedevere-bot commented Jun 29, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jeremycline commented Jun 28, 2018 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.