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

Remove redundant python includes, fix compiler warnings on re-exported symbols #2673

Merged
merged 2 commits into from Oct 16, 2021

Conversation

ankith26
Copy link
Contributor

Super minor PR, to hopefully fix #2613

Since pygame header already includes <Python.h> re-including those is redundant.
I am not a 100% sure that this fix works, because I believe python.h is header guarded anyways, but this PR is worth a shot

@ankith26
Copy link
Contributor Author

ankith26 commented Aug 15, 2021

Okay, I realise this PR does not fix the root cause of the issue, but I think I now know how to fix it

Yes, I fixed the issue in the latest commit.

Edit Starbuck: Ankith gave me this as evidence that this PR works as intended. https://stackoverflow.com/questions/37596516/warning-lnk4197-export-pyinit-python-module-name-specified-multiple-times-us/58866779#58866779

@ankith26 ankith26 changed the title Remove redundant python includes, fix compiler warnings Remove redundant python includes, fix compiler warnings on re-exported symbols Aug 15, 2021
@ankith26 ankith26 added this to the 2.0.2 milestone Aug 15, 2021
Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Thanks :) 🎉

@illume
Copy link
Member

illume commented Oct 8, 2021

I'd prefer to not merge this right now. To instead wait for a 2.0.3 dev release so it can get some wider testing. It's a little magical... and would it affect anything like pyinstaller I wonder?

@illume illume modified the milestones: 2.0.2, 2.0.3 Oct 8, 2021
@ankith26
Copy link
Contributor Author

ankith26 commented Oct 9, 2021

Yeah sure, this can totally wait for 2.0.3. I agree the fix is a bit hacky, and this sort of thing is best not implemented just before a release. Also the issue that it fixes shows up rarely, so it should be fine :)

@ankith26
Copy link
Contributor Author

Maybe this PR can be merged in now? I don't think this sort of thing should break with pyinstaller in any way, because those init symbols of the C extensions are still being exported correctly (this PR just fixes the warnings that happen when the export is attempted for a second time because of distutils passing args, and it is presumably a bug in the compiler that causes the crash in these warnings)

Also since distutils is deprecated, we can safely do this kind of hack because no one is going to touch distutils codebase and change the function names and such (it has been the same since py 2.7 xD)

This hack will hopefully not be required when we migrate from distutils, tho

@illume
Copy link
Member

illume commented Oct 16, 2021

Yeah, let's merge it in now so it can get some testing.

@illume illume merged commit 867ce99 into main Oct 16, 2021
@illume illume deleted the multiple-includes branch October 16, 2021 08:41
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.

Init functions specified multiple times on windows builds
2 participants