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

Fix windows linking error for unicode modules with a dummy function #4125

Merged
merged 3 commits into from Apr 29, 2021

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Apr 17, 2021

The fix in https://bugs.python.org/issue39432 isn't quite right ("_name".encode("punycode") != "name".encode("punycode")) and thus the unicode_imports test is failing on Windows 3.8+. I don't think Python is accepting patches to distutils any more but I'm going to submit one of setuptools in the near future.

For now just apply the hack to all versions >3.5

Link to distutils PR pypa/distutils#38

This creates a dummy function with the alternative name so that the export doesn't fail.

The fix in https://bugs.python.org/issue39432 isn't quite right (`"_name".encode("punycode") != "name".encode("punycode")`) and thus the unicode_imports test is failing on Windows 3.8+. I don't think Python is accepting patches to distutils any more but I'm going to submit one of setuptools in the near future.

For now just apply the hack to all versions >3.5
@scoder
Copy link
Contributor

scoder commented Apr 27, 2021

I'd be happier if there was a known limit to this. A hack that expands into the future indefinitely is always prone to failing at some point when something changes in the code that it works around. Especially since this is something that people will very rarely use and thus won't be noticed easily. Until it breaks.

@da-woods
Copy link
Contributor Author

An alternative hack would be to add a definition for the wrongly-named PyInitU function just as an empty dummy function, so that the linker finds it and is happy (and then keep the hack for Python <3.8.2 only)? That would have be less likely to break when/if distutils changes.

I don't think we could use the same hack pre-Python 3.8.2 though because it'd involve defined symbols with unicode names, which isn't supported all all C compilers.

@scoder
Copy link
Contributor

scoder commented Apr 27, 2021

Exporting an additional symbol (iff the users didn't disallow the export of the module init function name) seems like a simple and quite safe way to do this with minimal intrusion. Good idea.

A bug in distutils means that it tries to export a slightly
incorrectly named PyInitU function. Create a dummy function
with the alternative name so that the export doesn't fail.
@da-woods da-woods changed the title Enable fix_windows_unicode_modules more widely Fix windows linking error for unicode modules with a dummy function Apr 27, 2021
@da-woods
Copy link
Contributor Author

da-woods commented Apr 27, 2021

Right. I've done that. I haven't had a chance to test it on Windows so I don't think it should be merged until Appveyor's run on it. it seems to fix the unicode_imports test on Appveyor

@scoder scoder merged commit 7352104 into cython:master Apr 29, 2021
@scoder
Copy link
Contributor

scoder commented Apr 29, 2021

Thanks!

@scoder scoder added this to the 3.0 milestone Apr 29, 2021
@da-woods da-woods deleted the patch-2 branch April 29, 2021 06:35
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.

None yet

2 participants